fix: align ServiceManager direct paths, validate agent instances, assert multisig singleton - #335
Conversation
…ert multisig singleton ServiceManager - registerAgents() and unbond() now advance mapOperatorRegisterAgentsNonces and mapOperatorUnbondNonces, matching what the WithSignature variants already do. The two paths previously disagreed, so an authorization signed against the current nonce and never consumed stayed valid across later state changes. Closes registries #27 and #28. - Agent instance addresses at or below 0x0a are rejected before being forwarded to the registry, on both the direct and signature paths. Those addresses are either refused by Safe setup, leaving the service unable to deploy, or accepted while being unsignable. This is the accidental half of registries #29; the deliberate half, proof of control over an instance key, remains out of scope and OperatorWhitelist remains the intended control. GnosisSafeMultisig - create() re-reads masterCopy() on the new proxy and requires it to equal the pinned immutable. The factory is already given that immutable, so this is a post-condition on what was actually deployed rather than a change of behaviour for any valid creation. Comments in StakingBase and GnosisSafeSameAddressMultisig updated to match: the codehash check proves proxy bytecode, and singleton identity is established by each whitelisted implementation rather than assumed. forge build passes.
dagacha
left a comment
dagacha
left a comment
There was a problem hiding this comment.
Review done with Omp (Grok 4.7), medium effort.
Direct registerAgents and unbond advance mapOperatorRegisterAgentsNonces and mapOperatorUnbondNonces with the same (uint160(operator) | serviceId << 160) key the signature paths already use. The direct path keys on msg.sender. The increment is before the external calls, so a revert does not consume an outstanding authorization.
| uint96 public constant BOND_WRAPPER = 1; | ||
| // Highest address treated as reserved and rejected as an agent instance. | ||
| // Covers the zero address, Safe's SENTINEL_OWNERS (0x1) and the precompile range. | ||
| address public constant MAX_RESERVED_AGENT_INSTANCE = address(0x0a); |
There was a problem hiding this comment.
MAX_RESERVED_AGENT_INSTANCE is address(0x0a). The comment on lines 114–115 says this covers the precompile range. 0x0a is the Cancun point-evaluation precompile. This repo targets Prague (foundry.toml evm_version = "prague"). EIP-2537 precompiles 0x0b–0x11 still pass <= address(0x0a). Safe setupOwners accepts them and they cannot sign, which is the case item 29 describes for address(0x2) (docs/Vulnerabilities_list_registries.md).
Set the constant to address(0x11). State that it covers address(0), Safe's SENTINEL_OWNERS (0x1), and the Prague precompiles through 0x11. Do not claim L2 precompiles. Those sit outside this range, and item 29 already says a blocklist does not cover an arbitrary unsignable key.
There was a problem hiding this comment.
Correct, and I checked the live network rather than reason from the fork name — a call to an absent address returns empty, a precompile with bad input reverts:
| address | result |
|---|---|
0x0a – 0x11 |
all revert → precompiles present |
0x12, 0x13, 0x0100, 0x0101 |
return empty → nothing there |
So 0x0b–0x11 are live today and 0x0a was genuinely broken.
Set to 0xffff rather than 0x11. A bound that tracks the allocated range has to move every time a fork adds a precompile, and MAX_RESERVED_AGENT_INSTANCE is a constant — moving it means redeploying ServiceManager. Putting it far above the allocated range removes that maintenance entirely, and costs nothing: addresses are derived from hashes, so nothing below 2^16 can be controlled.
Also dropped the "L2 precompiles" phrasing. The comment now says the bound sits above the highest allocated one so it does not have to move, and why the range is safe to ban.
| // whatever it points at. Singleton identity is therefore established by the whitelisted multisig | ||
| // implementations themselves, each of which re-reads masterCopy() after creation and requires it | ||
| // to equal its own pinned immutable. Any implementation added to mapMultisigs must do the same; | ||
| // this codehash check is not a substitute for it. |
There was a problem hiding this comment.
Lines 808–811 say every whitelisted implementation re-reads masterCopy() after creation and requires it to equal its pinned immutable. That is not the code this PR leaves.
masterCopy() is re-read only in GnosisSafeMultisig.create (contracts/multisigs/GnosisSafeMultisig.sol:124-126). The other IMultisig.create implementations do not:
SafeMultisigWithRecoveryModule.createreturns the factory result with no post-check (contracts/multisigs/SafeMultisigWithRecoveryModule.sol:103). Its setup delegatecall target is the pinnedrecoveryModuleand the payload is onlyenableModule(:95-100), not caller-supplied code. A missing post-check there is defense in depth, not the same bug, and adding it would still not make "each implementation" true.RecoveryModule.createreusesmapServices[serviceId].multisigand never reads the singleton.PolySafeCreatorWithRecoveryModule.createchecks codehash, owners, and threshold, notmasterCopy().GnosisSafeSameAddressMultisigstill accepts a caller-supplied proxy and comparescodehashonly (contracts/multisigs/GnosisSafeSameAddressMultisig.sol:106-110). It is de-whitelisted on every supported chain (README.md:85-98), so that missing check is not current exposure.
Do not restore the previous note. Passing a pinned singleton into createProxyWithNonce does not survive a setup that writes slot 0. GnosisSafeMultisig passes caller-supplied to/payload into setup; the new post-creation masterCopy() check is the right control for that path. Item 24 still says the pinned immutable is enough (docs/Vulnerabilities_list_registries.md).
Rewrite these lines, and the copy in GnosisSafeSameAddressMultisig, so each path's actual guarantee is stated. Update item 24 to match. Keep the caller-supplied warning for any future mapMultisigs entry that accepts an existing proxy.
There was a problem hiding this comment.
Correct — I replaced a true note with a tidier false one, which is the worst way to edit a comment. Verified your list:
| implementation | masterCopy refs |
|---|---|
SafeMultisigWithRecoveryModule |
0 |
PolySafeCreatorWithRecoveryModule |
0 |
RecoveryModule |
0 |
GnosisSafeSameAddressMultisig |
1 — my own comment, not a call |
The comment now states what actually holds per path: the creating implementations read masterCopy() back against their own pinned singleton, the recovery-module creators rely on a pinned delegatecall target and payload, and the caller-supplied warning is kept for any future mapMultisigs entry.
| // whatever it points at. Singleton identity is therefore established by the whitelisted multisig | ||
| // implementations themselves, each of which re-reads masterCopy() after creation and requires it | ||
| // to equal its own pinned immutable. Any implementation added to mapMultisigs must do the same; | ||
| // this codehash check is not a substitute for it. |
There was a problem hiding this comment.
Same false claim as StakingBase.sol:808-811. This function still takes a caller-supplied proxy and does not call masterCopy(). Do not restore the deleted note verbatim: a pinned singleton passed to the factory is not a post-creation check, and item 24 still describes that older invariant.
This implementation is de-whitelisted (README.md:85-98), so the missing check is not exposure on supported deployments. The comment should say what this codehash check actually proves. Item 24 should record that a caller-supplied proxy still needs an explicit singleton check before it can be re-whitelisted.
There was a problem hiding this comment.
Rather than only rewrite the comment, GnosisSafeSameAddressMultisig.create() now performs the check — it reads masterCopy() off the provided proxy and requires it to equal a pinned singleton.
It had no singleton to compare against, so gnosisSafe is a second constructor argument, with deploy_09b, the L2 shell script and its verification encoding updated.
That makes the guarantee real on the one path that accepts a caller-supplied existing proxy, rather than documenting its absence — and it removes the reason this implementation could not be considered for re-whitelisting. Whether to re-enable it is a separate decision; this only removes the blocker.
…e same-address path ServiceManager - MAX_RESERVED_AGENT_INSTANCE moves from 0x0a to 0xffff. 0x0a was the top of the Cancun precompile range; this repo targets prague, where EIP-2537 adds precompiles through 0x11, and those were passing the check. Rather than track the allocated range fork by fork in a constant that needs a redeploy to change, the bound now sits far above it. Nothing in that range can be controlled, since addresses are derived from hashes. GnosisSafeSameAddressMultisig - create() now reads masterCopy() off the provided proxy and requires it to equal a pinned singleton. This is the one implementation that accepts a caller-supplied existing proxy, so the codehash check alone never established which singleton it delegates to. Adds gnosisSafe as a second constructor argument, with the deploy scripts and the verification encoding updated. StakingBase - the comment no longer claims every whitelisted implementation re-reads masterCopy(). It does not: the creating implementations do, while the recovery-module creators rely on a pinned delegatecall target and payload. The note now says that, and keeps the warning for any future entry that accepts a caller-supplied address. forge build passes.
…nstructor The singleton argument added in 4510de8 broke four before-each hooks, which CI caught and my forge build --skip test did not - the failures were in the hardhat suites, not compilation. ServiceRegistry, ServiceStaking, ServiceManagementWithOperatorSignatures and GnosisSafeSameAddressMultisig now pass gnosisSafe.address alongside the bytecode hash, and the zero-value constructor test gains a zero-address case for the new argument. 277 passing locally across the four suites.
The previous commit fixed the hardhat suites but not test/ServiceManagerMultisigBinding.t.sol, which builds its own adapter in _setupAdapter and so still called the one-argument constructor. The forge step compiles the test tree, so the build stayed red with 451 hardhat tests passing. Passes the local GnosisSafe master copy, which is the singleton behind agentSafe. forge test --match-contract Staking: 70 passed; PolySafeCreator: 1 passed; ServiceManagerMultisigBinding: 11 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The activity checker is an arbitrary external contract, chosen per instance by whoever deploys it, and checkpoint() calls it once per staked service. Each call now gets an explicit allowance rather than all the gas that remains. Both checker staticcalls in _checkRatioPass run under MAX_ACTIVITY_CHECKER_GAS, set to 100,000. The value is a ceiling and not a spend: measured in-EVM, the stock getMultisigNonces costs ~21k and a heavier custom checker ~16k, so a legitimate checker pays exactly what it paid before, and a call that does not return within the allowance is handled like any other failed call. VERSION 0.3.0 -> 0.4.0. forge --match-contract Staking: 70 passed. hardhat ServiceStaking + ServiceStakingFactory: 50 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
65d02f2 to
8a1063c
Compare
…ting The per-call gas allowance bounds the checker's execution but not the returndata the caller copies back afterwards. _activityStaticcall now copies at most a fixed number of bytes (an ABI uint256[] head plus up to 64 nonce words for getMultisigNonces, one word for isRatioPass) and treats a larger response as a failed call, so a checker cannot make the caller copy an unbounded buffer into memory. The nonces are parsed with _decodeUintArray rather than abi.decode: a malformed encoding returns not-valid instead of reverting, and the element count is derived from the buffer size rather than trusted from the buffer's own length word. The ratio bool is read as a raw word, with any non-canonical value scoring a fail. Together these keep the call unable to revert on the callee's behalf, whatever the checker returns. forge --match-contract Staking: 70 passed; PolySafeCreator: 1. hardhat ServiceStaking + ServiceStakingFactory: 50 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…efore create() accepts a caller-supplied proxy and then executes a caller-supplied payload against it, and that payload can be an execTransaction with operation = DelegateCall, which runs in the proxy's own storage and rewrites slot 0. The singleton check was running before the payload, so it validated a value the payload could then replace, and the getOwners()/getThreshold() checks that follow would delegate to the substituted singleton. The codehash cheap-reject stays early; the masterCopy() == gnosisSafe check now runs on the final state, after the payload. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dagacha
left a comment
dagacha
left a comment
There was a problem hiding this comment.
Approved. I found no confirmed Medium, High, or Must Fix issue in the current deployment.
Before considering re-whitelisting GnosisSafeSameAddressMultisig, please re-check masterCopy() after executing the supplied payload. The current check runs before that payload, and I reproduced a valid Safe transaction that changes the singleton afterward while create() still succeeds.
mariapiamo
left a comment
mariapiamo
left a comment
There was a problem hiding this comment.
Additional deployment-compatibility finding on 99b20dc, after checking the existing discussion. The main deployment and L2 shell updates are present; the remaining constructor callers are covered inline. Please update or explicitly retire those paths before merging.
Validation: both outdated argument patterns fail encoding against the compiled head ABI, while the two-argument control succeeds. The relevant 277 Hardhat tests pass.
| /// @param _proxyHash Approved multisig proxy hash. | ||
| constructor(bytes32 _proxyHash) { | ||
| /// @param _gnosisSafe Approved Gnosis Safe singleton address. | ||
| constructor(bytes32 _proxyHash, address _gnosisSafe) { |
There was a problem hiding this comment.
[P2] Update the remaining constructor callers. The main deployment and L2 shell paths have been updated as noted in the earlier reply. However, scripts/deployment/l2/deploy_06b_gnosis_safe_same_address_multisig.js still passes (proxyHash, { gasPrice }), scripts/deployment/l2/deploy_30_poly_safe_same_address_multisig.sh supplies only the proxy hash, and scripts/deployment/l2/verify_06_gnosis_safe_same_address_multisig.js still exports one argument. These remaining callers are incompatible with the new (bytes32, address) constructor. The JavaScript pattern fails with an invalid-address encoding error, and the one-argument pattern fails with a missing-constructor-argument error. Please update or explicitly retire these paths and their verification arguments, using the corresponding singleton for the PolySafe path.
There was a problem hiding this comment.
Fixed in 1d5fc1b. All three now pass gnosisSafe — the canonical Safe singleton the proxies delegate to, read from each globals file: deploy_06b.js passes parsedData.gnosisSafeAddress, verify_06.js exports it as the second arg, and deploy_30 (which deploys GnosisSafeSameAddressMultisig on the PolySafe path) reads gnosisSafeAddress and encodes constructor(bytes32,address). node --check clean on the JS, bash -n clean on the shell.
The (bytes32, address) constructor was picked up by deploy_09b and the 06b shell path, but three L2 callers still passed the old one-argument form and would fail at deployment: deploy_06b.js, verify_06.js and deploy_30 (which despite its name deploys GnosisSafeSameAddressMultisig on the PolySafe path). All three now supply gnosisSafe - the canonical Safe singleton the proxies delegate to, from each globals file - and the PolySafe verify encoding is constructor(bytes32,address). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1d5fc1b
4 participants
Three small changes to
ServiceManager, plus a post-condition in the Safe multisig creators, aStakingBaserefresh, and two comment fixes.ServiceManagerBoth register paths advance the same operator-nonce counters the
WithSignaturevariants already use, keeping the direct and signed paths in step. Closes registries #27 and #28, both Informative.Agent-instance addresses in a small reserved range are rejected before being forwarded to the registry, on both register paths. The bound is a
constantset well above any address that could be a real, signable key, so it needs no maintenance. This is the accidental half of registries #29; the deliberate half stays out of scope andOperatorWhitelistremains the intended control.Safe multisig creators
GnosisSafeMultisig.create()andGnosisSafeSameAddressMultisig.create()confirm, after creation, that the resulting proxy reports the pinned singleton. Both already hold that address, so this is a post-condition on what was deployed — no behaviour change for any valid creation.GnosisSafeSameAddressMultisignow takes the singleton as a constructor argument;deploy_09band the L2 script are updated.StakingBaseEach activity-checker call is given an explicit gas allowance rather than all remaining gas, so a single call's cost is bounded.
VERSION0.3.0 → 0.4.0.Comments
StakingBaseandGnosisSafeSameAddressMultisigcarried a note that no longer matched the code. Rewritten to state what each path actually guarantees.forge buildand the test suites pass. Functional tests for these changes live on a separate branch.