From ded3e5ae5ee5d01884f6f68aa851f1937581176e Mon Sep 17 00:00:00 2001 From: sh3ifu Date: Fri, 12 Jun 2026 18:05:19 +0300 Subject: [PATCH 1/2] test(R-I-18): proposal deadline-vs-timelock lower-bound regressions --- ethereum/test/MultisigProxy.t.sol | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/ethereum/test/MultisigProxy.t.sol b/ethereum/test/MultisigProxy.t.sol index 1be14db..6178a39 100644 --- a/ethereum/test/MultisigProxy.t.sol +++ b/ethereum/test/MultisigProxy.t.sol @@ -848,6 +848,55 @@ contract MultisigProxyTest is Test { proxy.proposeUpdateBridge(makeAddr('nb'), nonce, deadline, bitmap, sigs); } + function test_proposeUpdateBridge_revertsOnDeadlineBeforeTimelock() public { + uint256 nonce = proxy.proposalNonce(); + // One second short of the timelock window → dead on arrival. + uint256 deadline = block.timestamp + proxy.timelockDuration() - 1; + + bytes32 digest = MultisigHelper.digestProposeUpdateBridge(domainSep, makeAddr('nb'), nonce, deadline); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + vm.expectRevert(IMultisigProxy.DeadlineBeforeTimelock.selector); + proxy.proposeUpdateBridge(makeAddr('nb'), nonce, deadline, bitmap, sigs); + } + + function test_proposeUpdateBridge_acceptsDeadlineAtTimelockBoundary() public { + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + proxy.timelockDuration(); // exact boundary, `==` allowed + + bytes32 digest = MultisigHelper.digestProposeUpdateBridge(domainSep, makeAddr('nb'), nonce, deadline); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 proposalId = proxy.proposeUpdateBridge(makeAddr('nb'), nonce, deadline, bitmap, sigs); + assertEq( + uint8(proxy.getProposal(proposalId).status), + uint8(IMultisigProxy.ProposalStatus.Pending), + 'proposal at the exact boundary is created' + ); + } + + function test_proposeUpdateBridge_executableAtTimelockBoundary() public { + address newBridge = makeAddr('boundaryBridge'); + uint256 nonce = proxy.proposalNonce(); + uint256 timelock = proxy.timelockDuration(); + uint256 deadline = block.timestamp + timelock; // deadline == proposedAt + timelock + + bytes32 digest = MultisigHelper.digestProposeUpdateBridge(domainSep, newBridge, nonce, deadline); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 proposalId = proxy.proposeUpdateBridge(newBridge, nonce, deadline, bitmap, sigs); + + // Execute at the single instant where block.timestamp == proposedAt + + // timelock == deadline: timelock has just elapsed and the deadline has + // not yet passed (both checks use strict comparisons). + vm.warp(block.timestamp + timelock); + proxy.executeProposal(proposalId, abi.encode(newBridge)); + assertEq(proxy.bridge(), newBridge, 'boundary proposal executes at the exact instant'); + } + // ======================================================================== // Propose + Execute — UpdateEnclaveSigners // ======================================================================== From 85b224a2d8de2b21543255ff2e240af3a0892464 Mon Sep 17 00:00:00 2001 From: Denys <53053282+sh3ifu@users.noreply.github.com> Date: Wed, 17 Jun 2026 15:15:53 +0300 Subject: [PATCH 2/2] fix(R-I-11): cap destinationAddress / sourceAddress length in Bridge (#55) * fix(R-I-11): cap destinationAddress / sourceAddress length in Bridge * test(R-I-11): destinationAddress / sourceAddress length-cap regressions (#56) * test(R-I-11): destinationAddress / sourceAddress length-cap regressions * fix(R-I-12): cap proof length in Bridge.fundsOut (#57) * fix(R-I-12): cap proof length in Bridge.fundsOut * test(R-I-12): proof length-cap regressions for fundsOut (#58) * test(R-I-12): proof length-cap regressions for fundsOut * fix(R-W-12): enforce strict-majority signer threshold floor (#60) * fix(R-W-12): enforce strict-majority signer threshold floor * test(R-W-12): strict-majority signer threshold regressions (#61) * test(R-W-12): strict-majority signer threshold regressions * fix(R-W-14): require disjoint enclave and federation signer sets (#62) * fix(R-W-14): require disjoint enclave and federation signer sets * test(R-W-14): disjoint signer-set regressions (#63) * test(R-W-14): disjoint signer-set regressions * fix(R-I-06): cap signer-set size at MAX_SIGNERS (#64) * fix(R-I-06): cap signer-set size at MAX_SIGNERS * test(R-I-06): signer-set size cap regressions (#65) --- ethereum/src/Bridge.sol | 22 ++ ethereum/src/MultisigProxy.sol | 49 +++- ethereum/src/interfaces/IBridge.sol | 15 +- ethereum/src/interfaces/IMultisigProxy.sol | 2 + ethereum/test/Bridge.t.sol | 134 ++++++++++ ethereum/test/MultisigProxy.t.sol | 292 +++++++++++++++++++-- 6 files changed, 477 insertions(+), 37 deletions(-) diff --git a/ethereum/src/Bridge.sol b/ethereum/src/Bridge.sol index 3fc3eb5..c4cf0d0 100644 --- a/ethereum/src/Bridge.sol +++ b/ethereum/src/Bridge.sol @@ -40,6 +40,21 @@ contract Bridge is BridgeBase, IBridge, ReentrancyGuard { // State // ========================================================================= + /// @notice Upper bound on the `destinationAddress` (fundsIn) and + /// `sourceAddress` (fundsOut) byte length. These strings are echoed + /// into event logs, so an unbounded value would let a caller inflate + /// log-storage and indexer cost. 512 bytes covers every supported + /// destination representation (RGB invoices and other chains) with + /// ample headroom. + uint256 public constant MAX_ADDRESS_LENGTH = 512; + + /// @notice Upper bound on the `fundsOut` `proof` byte length. `proof` is + /// forwarded to the route verifier, so an unbounded blob would let a + /// release inflate calldata + verifier gas. 1024 bytes comfortably + /// fits the verifier formats (RGB is 64 bytes today; a future SPV + /// inclusion proof stays well under this). + uint256 public constant MAX_PROOF_LENGTH = 1024; + /// @notice CommissionManager that receives and custodies protocol fees. ICommissionManager public immutable commissionManager; @@ -203,6 +218,10 @@ contract Bridge is BridgeBase, IBridge, ReentrancyGuard { if (recipient == address(0)) revert InvalidRecipientAddress(); if (sourceChainId == 0) revert InvalidSourceChainId(); if (destinationChainId == 0) revert InvalidDestinationChainId(); + if (bytes(sourceAddress).length > MAX_ADDRESS_LENGTH) { + revert AddressTooLong(bytes(sourceAddress).length, MAX_ADDRESS_LENGTH); + } + if (proof.length > MAX_PROOF_LENGTH) revert ProofTooLong(proof.length, MAX_PROOF_LENGTH); if (amount > IERC20(TOKEN).balanceOf(address(this))) revert AmountExceedBridgePool(); // Common replay guard. Set the flag before any external interaction @@ -290,6 +309,9 @@ contract Bridge is BridgeBase, IBridge, ReentrancyGuard { ) private { if (amount < minFundsInAmount) revert AmountBelowMinimum(amount, minFundsInAmount); if (bytes(destinationAddress).length == 0) revert InvalidDestinationAddress(); + if (bytes(destinationAddress).length > MAX_ADDRESS_LENGTH) { + revert AddressTooLong(bytes(destinationAddress).length, MAX_ADDRESS_LENGTH); + } if (sourceChainId == 0) revert InvalidSourceChainId(); if (destinationChainId == 0) revert InvalidDestinationChainId(); diff --git a/ethereum/src/MultisigProxy.sol b/ethereum/src/MultisigProxy.sol index faf9102..76e872b 100644 --- a/ethereum/src/MultisigProxy.sol +++ b/ethereum/src/MultisigProxy.sol @@ -72,6 +72,12 @@ contract MultisigProxy is IMultisigProxy { /// @notice Hard upper bound on `executeBatch` size to keep gas use bounded. uint256 public constant MAX_BATCH_SIZE = 3; + /// @notice Hard upper bound on the size of either signer set. Bounds the + /// O(N^2) duplicate/disjointness checks and stays far within the + /// `uint256` signature-bitmap width (so no signer index is ever + /// unreachable). 20 is ample for an enclave/federation committee. + uint256 public constant MAX_SIGNERS = 20; + /// @notice Length of a Solidity function selector (`bytes4`) in bytes. Used /// to guard `callData` against payloads too short to even carry a /// selector before it is sliced via `calldataload`. @@ -183,9 +189,9 @@ contract MultisigProxy is IMultisigProxy { if (bridge_ == address(0)) revert ZeroBridge(); if (commissionManager_ == address(0)) revert ZeroCommissionManager(); if (enclaveSigners_.length == 0) revert NoSigners(); - if (enclaveThreshold_ == 0 || enclaveThreshold_ > enclaveSigners_.length) revert InvalidThreshold(); + _requireValidThreshold(enclaveThreshold_, enclaveSigners_.length); if (federationSigners_.length == 0) revert NoSigners(); - if (federationThreshold_ == 0 || federationThreshold_ > federationSigners_.length) revert InvalidThreshold(); + _requireValidThreshold(federationThreshold_, federationSigners_.length); if (commissionRecipient_ == address(0)) revert ZeroCommissionRecipient(); if (minTimelock_ == 0 || minTimelock_ >= MAX_PROPOSAL_LIFETIME) revert InvalidMinTimelock(); if (timelockDuration_ < minTimelock_) revert TimelockTooShort(); @@ -193,6 +199,9 @@ contract MultisigProxy is IMultisigProxy { _validateSigners(enclaveSigners_); _validateSigners(federationSigners_); + // The enclave and federation are distinct trust domains; an address in + // both would count toward quorum in each, collapsing that separation. + _requireDisjoint(enclaveSigners_, federationSigners_); bridge = bridge_; commissionManager = commissionManager_; @@ -869,8 +878,9 @@ contract MultisigProxy is IMultisigProxy { } else if (opType == OperationType.UpdateEnclaveSigners) { (address[] memory newSigners, uint256 newThreshold) = abi.decode(opData, (address[], uint256)); if (newSigners.length == 0) revert NoSigners(); - if (newThreshold == 0 || newThreshold > newSigners.length) revert InvalidThreshold(); + _requireValidThreshold(newThreshold, newSigners.length); _validateSigners(newSigners); + _requireDisjoint(newSigners, _federationSigners); _enclaveSigners = newSigners; enclaveThreshold = newThreshold; emit EnclaveSignersUpdated(newSigners, newThreshold); @@ -878,8 +888,9 @@ contract MultisigProxy is IMultisigProxy { } else if (opType == OperationType.UpdateFederationSigners) { (address[] memory newSigners, uint256 newThreshold) = abi.decode(opData, (address[], uint256)); if (newSigners.length == 0) revert NoSigners(); - if (newThreshold == 0 || newThreshold > newSigners.length) revert InvalidThreshold(); + _requireValidThreshold(newThreshold, newSigners.length); _validateSigners(newSigners); + _requireDisjoint(newSigners, _enclaveSigners); _federationSigners = newSigners; federationThreshold = newThreshold; emit FederationSignersUpdated(newSigners, newThreshold); @@ -1078,7 +1089,37 @@ contract MultisigProxy is IMultisigProxy { // ========================================================================= /// @dev Validates no zero addresses and no duplicates. O(n^2), fine for <20 signers. + /// @dev Enforces the quorum policy shared by both signer sets (enclave and + /// federation), in the constructor and the signer-update handlers: + /// - at least 2 signers (no single-key set); + /// - threshold of at least 2 (no single signer can authorise alone); + /// - threshold a strict majority (`2 * threshold > n`), which rejects + /// 1-of-N and any sub-majority quorum; + /// - threshold not exceeding the signer count. + /// The smallest valid sets are 2-of-2 and 2-of-3. This upholds the + /// spec invariant "one signer MUST NOT authorise fundsOut alone". + function _requireValidThreshold(uint256 threshold, uint256 n) private pure { + if (n < 2 || threshold < 2 || threshold > n || 2 * threshold <= n) { + revert InvalidThreshold(); + } + } + + /// @dev Reverts if any address in `candidate` also appears in `counterpart`. + /// Used to keep the enclave and federation signer sets disjoint so the + /// two trust domains stay independent (R-W-14). O(n*m), bounded by the + /// signer-set sizes. + function _requireDisjoint(address[] memory candidate, address[] memory counterpart) private pure { + for (uint256 i = 0; i < candidate.length; i++) { + for (uint256 j = 0; j < counterpart.length; j++) { + if (candidate[i] == counterpart[j]) revert SignerSetsOverlap(candidate[i]); + } + } + } + function _validateSigners(address[] memory signers) private pure { + // Bound the set before the O(N^2) duplicate scan below, and keep every + // signer index within the uint256 signature bitmap (R-I-06). + if (signers.length > MAX_SIGNERS) revert TooManySigners(signers.length, MAX_SIGNERS); for (uint256 i = 0; i < signers.length; i++) { if (signers[i] == address(0)) revert ZeroAddressSigner(); for (uint256 j = i + 1; j < signers.length; j++) { diff --git a/ethereum/src/interfaces/IBridge.sol b/ethereum/src/interfaces/IBridge.sol index 7bc1397..7d39dc0 100644 --- a/ethereum/src/interfaces/IBridge.sol +++ b/ethereum/src/interfaces/IBridge.sol @@ -9,22 +9,11 @@ interface IBridge { error InvalidDestinationAddress(); error InvalidDestinationChainId(); error InvalidSourceChainId(); - /// @notice `fundsOut` was called with `amount == 0`. A zero-amount release - /// is a no-op that only emits a redundant event, so it is rejected. - /// (The inbound path is guarded by `AmountBelowMinimum` instead, - /// since its non-zero `minFundsInAmount` already excludes zero.) error ZeroAmount(); - /// @notice A `fundsIn` deposit was below the configured `minFundsInAmount`. - /// The minimum is always non-zero, so this also rejects zero-amount - /// deposits, and it keeps dust — small enough that its commission - /// rounds to zero and only adds event/processing noise — off the - /// inbound path. Does not apply to `fundsOut` (authorized releases - /// of already-recorded amounts only check `amount > 0`). error AmountBelowMinimum(uint256 amount, uint256 minimum); - /// @notice `minFundsInAmount` was set to zero at construction or via - /// `setMinFundsInAmount`. The floor must be non-zero so that it - /// meaningfully rejects zero-amount and dust deposits. error InvalidMinFundsInAmount(); + error AddressTooLong(uint256 length, uint256 maxLength); + error ProofTooLong(uint256 length, uint256 maxLength); error InvalidRouteRegistryAddress(); error InvalidCommissionManagerAddress(); error NotLZAdapter(); diff --git a/ethereum/src/interfaces/IMultisigProxy.sol b/ethereum/src/interfaces/IMultisigProxy.sol index fe7bf97..915ccd0 100644 --- a/ethereum/src/interfaces/IMultisigProxy.sol +++ b/ethereum/src/interfaces/IMultisigProxy.sol @@ -63,6 +63,8 @@ interface IMultisigProxy { error InvalidSignature(); error ZeroAddressSigner(); error DuplicateSigner(); + error SignerSetsOverlap(address signer); + error TooManySigners(uint256 count, uint256 max); error CallFailed(); error UnknownOperationType(); error ZeroRecipient(); diff --git a/ethereum/test/Bridge.t.sol b/ethereum/test/Bridge.t.sol index b438b15..0b893ab 100644 --- a/ethereum/test/Bridge.t.sol +++ b/ethereum/test/Bridge.t.sol @@ -154,6 +154,15 @@ contract BridgeTest is Test { return abi.encode(ids); } + /// @dev Build an ASCII string of exactly `len` bytes (for address-length caps). + function _str(uint256 len) internal pure returns (string memory) { + bytes memory b = new bytes(len); + for (uint256 i = 0; i < len; i++) { + b[i] = 'a'; + } + return string(b); + } + function _setFundsInTokenRule(uint256 percent) internal { vm.prank(deployer); cm.setCommissionRule( @@ -435,6 +444,131 @@ contract BridgeTest is Test { bridge.fundsIn(999, SOURCE_CHAIN_ID, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); } + function test_fundsIn_revertsOnDestinationAddressTooLong() public { + uint256 max = bridge.MAX_ADDRESS_LENGTH(); + string memory tooLong = _str(max + 1); + + vm.expectRevert(abi.encodeWithSelector(IBridge.AddressTooLong.selector, max + 1, max)); + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, tooLong, TX_ID, ''); + } + + function test_fundsIn_acceptsDestinationAddressAtMaxLength() public { + uint256 max = bridge.MAX_ADDRESS_LENGTH(); + + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, _str(max), TX_ID, ''); + assertEq(rgbModule.fundsInRecords(TX_ID), AMOUNT, 'deposit at the address-length cap is accepted'); + } + + function test_fundsOut_revertsOnSourceAddressTooLong() public { + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); + + uint256 max = bridge.MAX_ADDRESS_LENGTH(); + string memory tooLong = _str(max + 1); + + vm.expectRevert(abi.encodeWithSelector(IBridge.AddressTooLong.selector, max + 1, max)); + vm.prank(multisig); + bridge.fundsOut( + recipient, AMOUNT, BURN_ID, + RGB_CHAIN_ID, SOURCE_CHAIN_ID, tooLong, + _proof(), _settlement(_singleFundsInId()) + ); + } + + function test_fundsOut_acceptsSourceAddressAtMaxLength() public { + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); + + uint256 max = bridge.MAX_ADDRESS_LENGTH(); + + vm.prank(multisig); + bridge.fundsOut( + recipient, AMOUNT, BURN_ID, + RGB_CHAIN_ID, SOURCE_CHAIN_ID, _str(max), + _proof(), _settlement(_singleFundsInId()) + ); + assertEq(usdt0.balanceOf(recipient), AMOUNT, 'release with sourceAddress at the cap succeeds'); + } + + // ======================================================================== + // Proof length cap (R-I-12) + // + // fundsOut forwards `proof` to the route verifier, so it is capped at + // MAX_PROOF_LENGTH to bound calldata + verifier gas. The exact cap is + // accepted; one byte over reverts ProofTooLong. The real RGB proof + // (abi.encode(uint256, bytes32) = 64 bytes) is far under the cap. + // ======================================================================== + + /// @dev Build a `bytes` blob of exactly `len` bytes. + function _bytesOfLength(uint256 len) internal pure returns (bytes memory b) { + b = new bytes(len); + for (uint256 i = 0; i < len; i++) { + b[i] = 0x61; + } + } + + function test_fundsOut_revertsOnProofTooLong() public { + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); + + uint256 max = bridge.MAX_PROOF_LENGTH(); + bytes memory tooLong = _bytesOfLength(max + 1); + + vm.expectRevert(abi.encodeWithSelector(IBridge.ProofTooLong.selector, max + 1, max)); + vm.prank(multisig); + bridge.fundsOut( + recipient, AMOUNT, BURN_ID, + RGB_CHAIN_ID, SOURCE_CHAIN_ID, SRC_ADDR, + tooLong, _settlement(_singleFundsInId()) + ); + } + + function test_fundsOut_acceptsProofAtMaxLength() public { + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); + + // A proof at the exact cap passes the length guard. It then reverts in + // the verifier (the blob is not a valid (height, commitment) pair), so + // the cap check is isolated by asserting it is NOT ProofTooLong: the + // call reaches the verifier instead. + uint256 max = bridge.MAX_PROOF_LENGTH(); + bytes memory atMax = _bytesOfLength(max); + + vm.prank(multisig); + try bridge.fundsOut( + recipient, AMOUNT, BURN_ID, + RGB_CHAIN_ID, SOURCE_CHAIN_ID, SRC_ADDR, + atMax, _settlement(_singleFundsInId()) + ) { + // a decodable proof would succeed; this blob won't, so we don't + // expect to land here — but if a future verifier accepts it, the + // length guard still passed, which is what this test asserts. + } catch (bytes memory reason) { + // Must NOT be the length guard — proving max-length passes it. + bytes4 sel = bytes4(reason); + assertTrue(sel != IBridge.ProofTooLong.selector, 'max-length proof must clear the length guard'); + } + } + + function test_fundsOut_acceptsRealProofUnderCap() public { + // The production-shaped 64-byte RGB proof is well under the cap and the + // happy path still succeeds. + vm.prank(user); + bridge.fundsIn(AMOUNT, RGB_CHAIN_ID, DST_ADDR, TX_ID, ''); + + assertLt(_proof().length, bridge.MAX_PROOF_LENGTH(), 'sanity: real proof under cap'); + + vm.prank(multisig); + bridge.fundsOut( + recipient, AMOUNT, BURN_ID, + RGB_CHAIN_ID, SOURCE_CHAIN_ID, SRC_ADDR, + _proof(), _settlement(_singleFundsInId()) + ); + assertEq(usdt0.balanceOf(recipient), AMOUNT, 'release with a normal proof succeeds'); + } + // ======================================================================== // fundsIn — adapter overload (`onlyLZAdapter`) // ======================================================================== diff --git a/ethereum/test/MultisigProxy.t.sol b/ethereum/test/MultisigProxy.t.sol index 6178a39..8bc3a2f 100644 --- a/ethereum/test/MultisigProxy.t.sol +++ b/ethereum/test/MultisigProxy.t.sol @@ -222,6 +222,20 @@ contract MultisigProxyTest is Test { ids[0] = TX_ID; } + /// @dev Valid strict-majority signer sets (2-of-2) used as filler in + /// constructor-revert tests that target a non-threshold revert. + function _validEnc() internal view returns (address[] memory a) { + a = new address[](2); + a[0] = encA1; + a[1] = encA2; + } + + function _validFed() internal view returns (address[] memory a) { + a = new address[](2); + a[0] = fedA1; + a[1] = fedA2; + } + function _fundsOutCalldata() internal view returns (bytes memory) { bytes memory proof = abi.encode(BLOCK_HEIGHT, COMMITMENT_HASH); bytes memory settlementData = abi.encode(_fundsInIds()); @@ -284,44 +298,34 @@ contract MultisigProxyTest is Test { } function test_constructor_revertsOnZeroCommission() public { - address[] memory enc = new address[](1); enc[0] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; vm.expectRevert(IMultisigProxy.ZeroCommissionRecipient.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, address(0), TIMELOCK, MIN_TIMELOCK); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _validFed(), 2, address(0), TIMELOCK, MIN_TIMELOCK); } function test_constructor_revertsOnTimelockTooLong() public { - address[] memory enc = new address[](1); enc[0] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; vm.expectRevert(IMultisigProxy.TimelockTooLong.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, 30 days, MIN_TIMELOCK); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _validFed(), 2, commissionReceiver, 30 days, MIN_TIMELOCK); } // ---- R-W-11: MIN_TIMELOCK floor (post-fix) ---- /// @dev UT-FIX-13: deploying with a timelock below the requested floor reverts. function test_constructor_revertsOnTimelockBelowMinTimelock() public { - address[] memory enc = new address[](1); enc[0] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; // timelock (1h) is below the requested floor (2h) -> TimelockTooShort vm.expectRevert(IMultisigProxy.TimelockTooShort.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, 1 hours, 2 hours); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _validFed(), 2, commissionReceiver, 1 hours, 2 hours); } /// @dev A zero floor is rejected — it would defeat the purpose of the fix. function test_constructor_revertsOnZeroMinTimelock() public { - address[] memory enc = new address[](1); enc[0] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; vm.expectRevert(IMultisigProxy.InvalidMinTimelock.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, TIMELOCK, 0); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _validFed(), 2, commissionReceiver, TIMELOCK, 0); } /// @dev A floor at/above the upper bound leaves no valid range — rejected. function test_constructor_revertsOnMinTimelockTooLong() public { - address[] memory enc = new address[](1); enc[0] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; vm.expectRevert(IMultisigProxy.InvalidMinTimelock.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, TIMELOCK, 30 days); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _validFed(), 2, commissionReceiver, TIMELOCK, 30 days); } function test_minTimelock_returnsConfiguredFloor() public view { @@ -329,17 +333,265 @@ contract MultisigProxyTest is Test { } function test_constructor_revertsOnDuplicateSigner() public { + // Duplicate enclave signer with an otherwise-valid 2-of-2 threshold, so + // the call clears the threshold guard and reverts in _validateSigners. address[] memory enc = new address[](2); enc[0] = encA1; enc[1] = encA1; - address[] memory fed = new address[](1); fed[0] = fedA1; vm.expectRevert(IMultisigProxy.DuplicateSigner.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + new MultisigProxy(address(bridge), address(cm), enc, 2, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); } function test_constructor_revertsOnZeroAddressSigner() public { - address[] memory enc = new address[](1); enc[0] = address(0); - address[] memory fed = new address[](1); fed[0] = fedA1; + // Zero-address enclave signer in a 2-signer set with a valid 2-of-2 + // threshold, so the call clears the threshold guard and reverts in + // _validateSigners. + address[] memory enc = new address[](2); enc[0] = address(0); enc[1] = encA2; vm.expectRevert(IMultisigProxy.ZeroAddressSigner.selector); - new MultisigProxy(address(bridge), address(cm), enc, 1, fed, 1, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + new MultisigProxy(address(bridge), address(cm), enc, 2, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + // ======================================================================== + // Strict-majority signer threshold floor (R-W-12 / UT-FIX-14) + // + // Both signer sets, at construction and on signer-update, must be a strict + // majority of at least 2 signers: n >= 2, threshold >= 2, threshold <= n, + // 2*threshold > n. Rejects 1-of-N, the degenerate 1-of-1, and sub-majority + // quorums; the smallest valid sets are 2-of-2 and 2-of-3. Signer-update + // validation runs at executeProposal time (in _executeByType), so the bad + // proposal is created successfully and reverts only on execution. + // ======================================================================== + + /// @dev Build an n-element signer array from a seed (distinct non-zero addrs). + function _signers(uint256 n) internal pure returns (address[] memory a) { + a = new address[](n); + for (uint256 i = 0; i < n; i++) { + a[i] = address(uint160(0xA000 + i)); + } + } + + /// @dev A second n-element signer array disjoint from `_signers` — used as + /// the counterpart set so the two trust domains do not overlap. + function _signersB(uint256 n) internal pure returns (address[] memory a) { + a = new address[](n); + for (uint256 i = 0; i < n; i++) { + a[i] = address(uint160(0xB000 + i)); + } + } + + // ---- Constructor ---- + + function test_constructor_rejectsOneOfThree_enclave() public { + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + new MultisigProxy(address(bridge), address(cm), _signers(3), 1, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + function test_constructor_rejectsOneOfOne_enclave() public { + // n < 2 — single-key set, rejected even though 2*1 > 1. + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + new MultisigProxy(address(bridge), address(cm), _signers(1), 1, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + function test_constructor_rejectsSubMajority_enclave() public { + // 2-of-4: 2*2 == 4, not a strict majority. + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + new MultisigProxy(address(bridge), address(cm), _signers(4), 2, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + function test_constructor_rejectsOneOfThree_federation() public { + // Enclave valid, federation 1-of-3 — the floor applies to both sets. + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + new MultisigProxy(address(bridge), address(cm), _validEnc(), 2, _signers(3), 1, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + function test_constructor_acceptsTwoOfTwo() public { + MultisigProxy p = new MultisigProxy( + address(bridge), address(cm), _signers(2), 2, _signersB(2), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK + ); + assertEq(p.enclaveThreshold(), 2); + assertEq(p.federationThreshold(), 2); + } + + function test_constructor_acceptsThreeOfFour() public { + // Strict majority with a non-trivial set (2*3 > 4). + MultisigProxy p = new MultisigProxy( + address(bridge), address(cm), _signers(4), 3, _signersB(4), 3, commissionReceiver, TIMELOCK, MIN_TIMELOCK + ); + assertEq(p.enclaveThreshold(), 3); + assertEq(p.federationThreshold(), 3); + } + + // ---- Signer update (validated at executeProposal) ---- + + function test_proposeUpdateEnclaveSigners_rejectsOneOfNAtExecute() public { + address[] memory newSigners = _signers(3); + uint256 badThreshold = 1; // 1-of-3 + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateEnclaveSigners( + domainSep, newSigners, badThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + // Propose succeeds — threshold validity is enforced on execution. + bytes32 id = proxy.proposeUpdateEnclaveSigners(newSigners, badThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + proxy.executeProposal(id, abi.encode(newSigners, badThreshold)); + } + + function test_proposeUpdateFederationSigners_rejectsSubMajorityAtExecute() public { + address[] memory newSigners = _signers(4); + uint256 badThreshold = 2; // 2-of-4, sub-majority + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateFederationSigners( + domainSep, newSigners, badThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 id = proxy.proposeUpdateFederationSigners(newSigners, badThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + vm.expectRevert(IMultisigProxy.InvalidThreshold.selector); + proxy.executeProposal(id, abi.encode(newSigners, badThreshold)); + } + + function test_proposeUpdateFederationSigners_acceptsStrictMajority() public { + address[] memory newSigners = _signers(3); + uint256 newThreshold = 2; // 2-of-3 + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateFederationSigners( + domainSep, newSigners, newThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 id = proxy.proposeUpdateFederationSigners(newSigners, newThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + proxy.executeProposal(id, abi.encode(newSigners, newThreshold)); + assertEq(proxy.federationThreshold(), 2); + assertEq(proxy.getFederationSigners().length, 3); + } + + // ======================================================================== + // Disjoint signer sets (R-W-14 / UT-FIX-16) + // + // The enclave and federation sets must not share an address, at construction + // and on signer-update (validated at executeProposal). The setUp sets are + // already disjoint (encA* vs fedA*). + // ======================================================================== + + function test_constructor_rejectsOverlappingSignerSets() public { + // encA1 is present in both the enclave and the federation set. + address[] memory enc = new address[](2); enc[0] = encA1; enc[1] = encA2; + address[] memory fed = new address[](2); fed[0] = encA1; fed[1] = fedA2; + vm.expectRevert(abi.encodeWithSelector(IMultisigProxy.SignerSetsOverlap.selector, encA1)); + new MultisigProxy(address(bridge), address(cm), enc, 2, fed, 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK); + } + + function test_constructor_acceptsDisjointSignerSets() public { + MultisigProxy p = new MultisigProxy( + address(bridge), address(cm), _signers(2), 2, _signersB(2), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK + ); + assertEq(p.getEnclaveSigners().length, 2); + assertEq(p.getFederationSigners().length, 2); + } + + function test_proposeUpdateEnclaveSigners_rejectsOverlapWithFederationAtExecute() public { + // New enclave set includes fedA1, a current federation signer. + address[] memory newSigners = new address[](2); newSigners[0] = fedA1; newSigners[1] = encA2; + uint256 newThreshold = 2; + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateEnclaveSigners( + domainSep, newSigners, newThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 id = proxy.proposeUpdateEnclaveSigners(newSigners, newThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + vm.expectRevert(abi.encodeWithSelector(IMultisigProxy.SignerSetsOverlap.selector, fedA1)); + proxy.executeProposal(id, abi.encode(newSigners, newThreshold)); + } + + function test_proposeUpdateFederationSigners_rejectsOverlapWithEnclaveAtExecute() public { + // New federation set includes encA1, a current enclave signer. + address[] memory newSigners = new address[](2); newSigners[0] = encA1; newSigners[1] = fedA2; + uint256 newThreshold = 2; + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateFederationSigners( + domainSep, newSigners, newThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 id = proxy.proposeUpdateFederationSigners(newSigners, newThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + vm.expectRevert(abi.encodeWithSelector(IMultisigProxy.SignerSetsOverlap.selector, encA1)); + proxy.executeProposal(id, abi.encode(newSigners, newThreshold)); + } + + // ======================================================================== + // Signer-set size cap (R-I-06 / UT-FIX-27) + // + // Either set is capped at MAX_SIGNERS, at construction and on signer-update + // (validated in _validateSigners at executeProposal). The cap bounds the + // O(N^2) scan and keeps every index within the uint256 signature bitmap. + // ======================================================================== + + function test_constructor_rejectsTooManySigners() public { + uint256 max = proxy.MAX_SIGNERS(); + address[] memory enc = _signers(max + 1); // 21 distinct enclave signers + uint256 encThreshold = (max + 1) / 2 + 1; // strict majority, so it clears the threshold guard + + vm.expectRevert(abi.encodeWithSelector(IMultisigProxy.TooManySigners.selector, max + 1, max)); + new MultisigProxy( + address(bridge), address(cm), enc, encThreshold, _validFed(), 2, commissionReceiver, TIMELOCK, MIN_TIMELOCK + ); + } + + function test_constructor_acceptsSignersAtMax() public { + uint256 max = proxy.MAX_SIGNERS(); + uint256 threshold = max / 2 + 1; // 11-of-20 strict majority + + MultisigProxy p = new MultisigProxy( + address(bridge), address(cm), _signers(max), threshold, _signersB(max), threshold, commissionReceiver, TIMELOCK, MIN_TIMELOCK + ); + assertEq(p.getEnclaveSigners().length, max); + assertEq(p.getFederationSigners().length, max); + } + + function test_proposeUpdateEnclaveSigners_rejectsTooManySignersAtExecute() public { + uint256 max = proxy.MAX_SIGNERS(); + address[] memory newSigners = _signers(max + 1); + uint256 newThreshold = (max + 1) / 2 + 1; // valid strict majority + uint256 nonce = proxy.proposalNonce(); + uint256 deadline = block.timestamp + 1 days; + + bytes32 digest = MultisigHelper.digestProposeUpdateEnclaveSigners( + domainSep, newSigners, newThreshold, nonce, deadline + ); + (uint256[] memory pks, uint256 bitmap) = _fedSigSet2of3(); + bytes[] memory sigs = MultisigHelper.signAll(vm, digest, pks); + + bytes32 id = proxy.proposeUpdateEnclaveSigners(newSigners, newThreshold, nonce, deadline, bitmap, sigs); + + vm.warp(block.timestamp + TIMELOCK + 1); + vm.expectRevert(abi.encodeWithSelector(IMultisigProxy.TooManySigners.selector, max + 1, max)); + proxy.executeProposal(id, abi.encode(newSigners, newThreshold)); } // ========================================================================