Skip to content

test(R-I-18): proposal deadline-vs-timelock lower-bound regressions #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sh3ifu merged 2 commits into from
Jun 17, 2026
Merged

test(R-I-18): proposal deadline-vs-timelock lower-bound regressions #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ded3e5a
test(R-I-18): proposal deadline-vs-timelock lower-bound regressions
sh3ifu Jun 12, 2026
85b224a
fix(R-I-11): cap destinationAddress / sourceAddress length in Bridge …
sh3ifu Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions ethereum/src/Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down
49 changes: 45 additions & 4 deletions ethereum/src/MultisigProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -183,16 +189,19 @@ 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();
if (timelockDuration_ >= MAX_PROPOSAL_LIFETIME) revert TimelockTooLong();

_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_;
Expand Down Expand Up @@ -869,17 +878,19 @@ 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);

} 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);
Expand Down Expand Up @@ -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++) {
Expand Down
15 changes: 2 additions & 13 deletions ethereum/src/interfaces/IBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions ethereum/src/interfaces/IMultisigProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
134 changes: 134 additions & 0 deletions ethereum/test/Bridge.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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`)
// ========================================================================
Expand Down
Loading