Skip to content

Chore/updated relayer encoding fix #430

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.

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

Draft
wants to merge 17 commits into
base: dev
Choose a base branch
from
Draft
18 changes: 4 additions & 14 deletions contracts/src/arbitrumToEth/VeaInboxArbToEth.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT

/// @custom:authors: [@jaybuidl, @shotaronowhere]
/// @custom:authors: [@jaybuidl, @mani99brar, @shotaronowhere]
/// @custom:reviewers: []
/// @custom:auditors: []
/// @custom:bounties: []
Expand Down Expand Up @@ -70,26 +70,16 @@ contract VeaInboxArbToEth is IVeaInbox {
/// Amortized cost is constant.
/// Note: See docs for details how inbox manages merkle tree state.
/// @param _to The address of the contract on the receiving chain which receives the calldata.
/// @param _fnSelector The function selector of the receiving contract.
/// @param _data The message calldata, abi.encode(param1, param2, ...)
/// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...)
/// @return msgId The zero based index of the message in the inbox.
function sendMessage(address _to, bytes4 _fnSelector, bytes memory _data) external override returns (uint64) {
function sendMessage(address _to, bytes calldata _data) external override returns (uint64) {
uint64 oldCount = count;

// Given arbitrum's speed limit of 7 million gas / second, it would take atleast 8 million years of full blocks to overflow.
// It *should* be impossible to overflow, but we check to be safe when appending to the tree.
require(oldCount < type(uint64).max, "Inbox is full.");

bytes memory nodeData = abi.encodePacked(
oldCount,
_to,
// _data is abi.encode(param1, param2, ...), we need to encode it again to get the correct leaf data
abi.encodePacked( // equivalent to abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
_fnSelector,
bytes32(uint256(uint160(msg.sender))), // big endian padded encoding of msg.sender, simulating abi.encodeWithSelector
_data
)
);
bytes memory nodeData = abi.encodePacked(oldCount, _to, msg.sender, _data);

// single hashed leaf
bytes32 newInboxNode = keccak256(nodeData);
Expand Down
26 changes: 22 additions & 4 deletions contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT

/// @custom:authors: [@jaybuidl, @shotaronowhere]
/// @custom:authors: [@jaybuidl, @mani99brar, @shotaronowhere]
/// @custom:reviewers: []
/// @custom:auditors: []
/// @custom:bounties: []
Expand Down Expand Up @@ -41,6 +41,7 @@ contract VeaOutboxArbToEth is IVeaOutboxOnL1 {

mapping(uint256 epoch => bytes32) public claimHashes; // epoch => claim
mapping(uint256 messageId => bytes32) internal relayed; // msgId/256 => packed replay bitmap, preferred over a simple boolean mapping to save 15k gas per message
mapping(address => mapping(address => bool)) public allowlist; // to => from => allowed, Enforces allowed sender addresses for a receiver contract. Address(0) allowing all senders.

uint256 public sequencerDelayLimit; // This is MaxTimeVariation.delaySeconds from the arbitrum sequencer inbox, it is the maximum seconds the sequencer can backdate L2 txns relative to the L1 clock.
SequencerDelayLimitDecreaseRequest public sequencerDelayLimitDecreaseRequest; // Decreasing the sequencerDelayLimit requires a delay to avoid griefing by sequencer, so we keep track of the request here.
Expand Down Expand Up @@ -351,15 +352,32 @@ contract VeaOutboxArbToEth is IVeaOutboxOnL1 {
emit Verified(_epoch);
}

/// @dev Sets the allowlist for the sender gateway.
/// Note: Address(0) is used to allow all addresses.
/// @param _from The address to allow or disallow
/// @param _allowed Whether to allow or disallow the address.
function setAllowlist(address _from, bool _allowed) external {
allowlist[msg.sender][_from] = _allowed;
}

/// @dev Verifies and relays the message. UNTRUSTED.
/// @param _proof The merkle proof to prove the message inclusion in the inbox state root.
/// @param _msgId The zero based index of the message in the inbox.
/// @param _to The address of the contract on Ethereum to call.
/// @param _message The message encoded in the vea inbox as abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
function sendMessage(bytes32[] calldata _proof, uint64 _msgId, address _to, bytes calldata _message) external {
/// @param _from The address of the contract on Arbitrum that sent the message.
/// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...)
function sendMessage(
bytes32[] calldata _proof,
uint64 _msgId,
address _to,
address _from,
bytes calldata _message
) external {
require(_proof.length < 64, "Proof too long.");
bool isAllowed = allowlist[_to][_from] || allowlist[_to][address(0)];
require(isAllowed, "Message sender not allowed to call receiver.");

bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _message));
bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _from, _message));

// double hashed leaf
// avoids second order preimage attacks
Expand Down
18 changes: 4 additions & 14 deletions contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT

/// @custom:authors: [@jaybuidl, @shotaronowhere]
/// @custom:authors: [@jaybuidl, @mani99brar, @shotaronowhere]
/// @custom:reviewers: []
/// @custom:auditors: []
/// @custom:bounties: []
Expand Down Expand Up @@ -70,26 +70,16 @@ contract VeaInboxArbToGnosis is IVeaInbox {
/// Amortized cost is constant.
/// Note: See docs for details how inbox manages merkle tree state.
/// @param _to The address of the contract on the receiving chain which receives the calldata.
/// @param _fnSelector The function selector of the receiving contract.
/// @param _data The message calldata, abi.encode(param1, param2, ...)
/// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...)
/// @return msgId The zero based index of the message in the inbox.
function sendMessage(address _to, bytes4 _fnSelector, bytes memory _data) external override returns (uint64) {
function sendMessage(address _to, bytes calldata _data) external override returns (uint64) {
uint64 oldCount = count;

// Given arbitrum's speed limit of 7 million gas / second, it would take atleast 8 million years of full blocks to overflow.
// It *should* be impossible to overflow, but we check to be safe when appending to the tree.
require(oldCount < type(uint64).max, "Inbox is full.");

bytes memory nodeData = abi.encodePacked(
oldCount,
_to,
// _data is abi.encode(param1, param2, ...), we need to encode it again to get the correct leaf data
abi.encodePacked( // equivalent to abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
_fnSelector,
bytes32(uint256(uint160(msg.sender))), // big endian padded encoding of msg.sender, simulating abi.encodeWithSelector
_data
)
);
bytes memory nodeData = abi.encodePacked(oldCount, _to, msg.sender, _data);

// single hashed leaf
bytes32 newInboxNode = keccak256(nodeData);
Expand Down
25 changes: 21 additions & 4 deletions contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT

/// @custom:authors: [@jaybuidl, @shotaronowhere]
/// @custom:authors: [@jaybuidl, @mani99brar, @shotaronowhere]
/// @custom:reviewers: []
/// @custom:auditors: []
/// @custom:bounties: []
Expand Down Expand Up @@ -42,6 +42,7 @@ contract VeaOutboxArbToGnosis is IVeaOutboxOnL1, ISequencerDelayUpdatable {

mapping(uint256 epoch => bytes32) public claimHashes; // epoch => claim
mapping(uint256 messageId => bytes32) internal relayed; // msgId/256 => packed replay bitmap, preferred over a simple boolean mapping to save 15k gas per message
mapping(address => mapping(address => bool)) public allowlist; // to => from => allowed, Enforces allowed sender addresses for a receiver contract. Address(0) allowing all senders.

uint256 public sequencerDelayLimit; // This is MaxTimeVariation.delaySeconds from the arbitrum sequencer inbox, it is the maximum seconds the sequencer can backdate L2 txns relative to the L1 clock.
uint256 public timestampDelayUpdated; // The timestamp of the last sequencer delay update.
Expand Down Expand Up @@ -295,15 +296,31 @@ contract VeaOutboxArbToGnosis is IVeaOutboxOnL1, ISequencerDelayUpdatable {
emit Verified(_epoch);
}

/// @dev Sets the allowlist for the sender gateway.
/// @param _from The address to allow or disallow
/// @param _allow Whether to allow or disallow the address.
function setAllowlist(address _from, bool _allow) external {
allowlist[msg.sender][_from] = _allow;
}

/// @dev Verifies and relays the message. UNTRUSTED.
/// @param _proof The merkle proof to prove the message inclusion in the inbox state root.
/// @param _msgId The zero based index of the message in the inbox.
/// @param _to The address of the contract on Gnosis to call.
/// @param _message The message encoded in the vea inbox as abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
function sendMessage(bytes32[] calldata _proof, uint64 _msgId, address _to, bytes calldata _message) external {
/// @param _from The address of the contract on Arbitrum that sent the message.
/// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...)
function sendMessage(
bytes32[] calldata _proof,
uint64 _msgId,
address _to,
address _from,
bytes calldata _message
) external {
require(_proof.length < 64, "Proof too long.");
bool isAllowed = allowlist[_to][_from] || allowlist[_to][address(0)];
require(isAllowed, "Message sender not allowed to call receiver.");

bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _message));
bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _from, _message));

// double hashed leaf
// avoids second order preimage attacks
Expand Down
16 changes: 3 additions & 13 deletions contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,26 +71,16 @@ contract VeaInboxGnosisToArb is IVeaInbox {
/// Amortized cost is constant.
/// Note: See docs for details how inbox manages merkle tree state.
/// @param _to The address of the contract on the receiving chain which receives the calldata.
/// @param _fnSelector The function selector of the receiving contract.
/// @param _data The message calldata, abi.encode(param1, param2, ...)
/// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...)
/// @return msgId The zero based index of the message in the inbox.
function sendMessage(address _to, bytes4 _fnSelector, bytes memory _data) external override returns (uint64) {
function sendMessage(address _to, bytes calldata _data) external override returns (uint64) {
uint64 oldCount = count;

// Given arbitrum's speed limit of 7 million gas / second, it would take atleast 8 million years of full blocks to overflow.
// It *should* be impossible to overflow, but we check to be safe when appending to the tree.
require(oldCount < type(uint64).max, "Inbox is full.");

bytes memory nodeData = abi.encodePacked(
oldCount,
_to,
// _data is abi.encode(param1, param2, ...), we need to encode it again to get the correct leaf data
abi.encodePacked( // equivalent to abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
_fnSelector,
bytes32(uint256(uint160(msg.sender))), // big endian padded encoding of msg.sender, simulating abi.encodeWithSelector
_data
)
);
bytes memory nodeData = abi.encodePacked(oldCount, _to, msg.sender, _data);

// single hashed leaf
bytes32 newInboxNode = keccak256(nodeData);
Expand Down
24 changes: 21 additions & 3 deletions contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ contract VeaOutboxGnosisToArb is IVeaOutboxOnL2 {
mapping(uint256 epoch => Claim) public claims; // epoch => claim
mapping(uint256 epoch => address) public challengers; // epoch => challenger
mapping(uint256 messageId => bytes32) internal relayed; // msgId/256 => packed replay bitmap, preferred over a simple boolean mapping to save 15k gas per message
mapping(address => mapping(address => bool)) public allowlist; // to => from => allowed

enum Party {
None,
Expand Down Expand Up @@ -281,15 +282,32 @@ contract VeaOutboxGnosisToArb is IVeaOutboxOnL2 {
emit Verified(_epoch);
}

/// @dev Sets the allowlist for the sender gateway.
/// Note: Address(0) is used to allow all addresses.
/// @param _from The address to allow or disallow
/// @param _allowed Whether to allow or disallow the address.
function setAllowlist(address _from, bool _allowed) external {
allowlist[msg.sender][_from] = _allowed;
}

/// @dev Verifies and relays the message. UNTRUSTED.
/// @param _proof The merkle proof to prove the message inclusion in the inbox state root.
/// @param _msgId The zero based index of the message in the inbox.
/// @param _to The address of the contract on Arbitrum to call.
/// @param _message The message encoded in the vea inbox as abi.encodeWithSelector(fnSelector, msg.sender, param1, param2, ...)
function sendMessage(bytes32[] memory _proof, uint64 _msgId, address _to, bytes memory _message) external {
/// @param _from The address of the message sender
/// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...)
Comment on lines +297 to +298
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Parameter documentation is incomplete.

The NatSpec comment for the _from parameter doesn't match the actual parameter name or purpose correctly.

Apply this diff to fix the documentation:

-    /// @param _from The address of the message sender
+    /// @param _from The address of the message sender on the source chain.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// @param _from The address of the message sender
/// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...)
/// @param _from The address of the message sender on the source chain.
/// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...)
🤖 Prompt for AI Agents
In contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol around lines 297 to
298, the NatSpec @param for `_from` is incorrect or misleading; update the
comment to use the correct parameter name and describe its actual purpose (e.g.,
the original sender/address that posted the message into the Vea inbox on the
source chain) and ensure the `_message` param line remains accurate (message
ABI-encoded with selector and parameters).

function sendMessage(
bytes32[] memory _proof,
uint64 _msgId,
address _to,
address _from,
bytes memory _message
) external {
require(_proof.length < 64, "Proof too long.");
bool isAllowed = allowlist[_to][_from] || allowlist[_to][address(0)];
require(isAllowed, "Message sender not allowed to call receiver.");

bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _message));
bytes32 nodeHash = keccak256(abi.encodePacked(_msgId, _to, _from, _message));

// double hashed leaf
// avoids second order preimage attacks
Expand Down
5 changes: 2 additions & 3 deletions contracts/src/interfaces/inboxes/IVeaInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ interface IVeaInbox {
/// @dev Sends an arbitrary message to receiving chain.
/// Note: Calls authenticated by receiving gateway checking the sender argument.
/// @param _to The cross-domain contract address which receives the calldata.
/// @param _fnSelection The function selector of the receiving contract.
/// @param _data The message calldata, abi.encode(...)
/// @param _data The message calldata, abi.encodeWithSelector(...)
/// @return msgId The index of the message in the inbox, as a message Id, needed to relay the message.
function sendMessage(address _to, bytes4 _fnSelection, bytes memory _data) external returns (uint64 msgId);
function sendMessage(address _to, bytes calldata _data) external returns (uint64 msgId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Locate and update all callers to the new 2-arg signature

Make sure internal tools, relayers, and tests no longer pass a separate selector/arg.


🏁 Script executed:

#!/bin/bash
# Find old-style 3-arg calls
rg -n -A1 -B2 $'sendMessage\\s*\\(\\s*[^,]+,\\s*[^,]+,\\s*[^)]*\\)' -g '!**/interfaces/**'

# List all sendMessage calls for manual review
rg -n -A1 -B2 $'sendMessage\\s*\\('

Length of output: 312375


Update all sendMessage invocations to drop the removed selector/from argument

The inbox sendMessage signature has been simplified to two parameters (address _to, bytes calldata _data), and the outbox sendMessage no longer takes a separate sender argument. Please remove the extra argument from every call and merge the former from (or selector) into the calldata.

Affected call sites:
• relay-cli/src/utils/relay.ts (around lines 61–64):

  • Change
    veaOutbox.sendMessage(proof, nonce, to, from, data)
    to
    veaOutbox.sendMessage(proof, nonce, to, dataWithSender)
    • contracts/test/integration/ArbToGnosis.ts (lines 304, 308, 366, 430, 769, 946):
  • Replace each
    …sendMessage(proof, nonce, to, from, msgData)
    with
    …sendMessage(proof, nonce, to, msgDataWithSender)
    • contracts/test/integration/ArbToEth.ts (lines 287, 292, 295, 348, 402, 407, 410, 463, 468, 541, 625, 847, 975):
  • Same 5-arg → 4-arg update
    • Search for any other .sendMessage( invocations with three or more comma-separated args (outside /interfaces) and refactor accordingly.

Ensure the passed bytes payload includes both selector and original sender where required.

🤖 Prompt for AI Agents
In contracts/src/interfaces/inboxes/IVeaInbox.sol around line 17, the
sendMessage signature was simplified to (address _to, bytes calldata _data);
update all call sites to remove the extra sender/selector argument and instead
embed the former from/selector into the bytes calldata payload. Specifically, in
relay-cli/src/utils/relay.ts (around lines 61–64) change
veaOutbox.sendMessage(proof, nonce, to, from, data) to
veaOutbox.sendMessage(proof, nonce, to, dataWithSender) where dataWithSender is
data prefixed or merged with the sender/selector; in
contracts/test/integration/ArbToGnosis.ts (lines ~304, 308, 366, 430, 769, 946)
and contracts/test/integration/ArbToEth.ts (lines ~287, 292, 295, 348, 402, 407,
410, 463, 468, 541, 625, 847, 975) replace each 5-arg call with the 4-arg
variant and construct msgDataWithSender by concatenating the selector/sender
into the bytes payload; also grep the repo for any other .sendMessage( calls
with 3+ comma-separated args (excluding /interfaces) and refactor them similarly
so the bytes payload contains selector and original sender where required.


/// @dev Snapshots can be saved a maximum of once per epoch.
/// Saves snapshot of state root.
Expand Down
17 changes: 15 additions & 2 deletions contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,32 @@ pragma solidity ^0.8.24;
import "../types/VeaClaim.sol";

/// @dev Interface of the Vea Outbox on L1 chains like Ethereum, Gnosis, Polygon POS where storage is expensive.

interface IVeaOutboxOnL1 {
/// @dev Verifies and relays the message.
/// Note: Gateways expect first argument of message call to be the arbitrum message sender, used for authentication.
/// @param _proof The merkle proof to prove the message.
/// @param _msgId The zero based index of the message in the inbox.
/// @param _to The address to send the message to.
/// @param _from The address of the contract on L2 that sent the message, used for authentication.
/// @param _message The message to relay.
function sendMessage(bytes32[] calldata _proof, uint64 _msgId, address _to, bytes calldata _message) external;
function sendMessage(
bytes32[] calldata _proof,
uint64 _msgId,
address _to,
address _from,
bytes calldata _message
) external;

/// @dev Resolves any challenge of the optimistic claim for 'epoch' using the canonical bridge.
/// Note: Access restricted to canonical bridge.
/// @param _epoch The epoch to verify.
/// @param _stateRoot The true state root for the epoch.
/// @param _claim The claim associated with the epoch.
function resolveDisputedClaim(uint256 _epoch, bytes32 _stateRoot, Claim memory _claim) external;

/// @dev Sets the allowlist for the sender gateway.
/// Note: Address(0) is used to allow all addresses.
/// @param _from The address to allow or disallow
/// @param _allow Whether to allow or disallow the address.
function setAllowlist(address _from, bool _allow) external;
}
15 changes: 14 additions & 1 deletion contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,25 @@ interface IVeaOutboxOnL2 {
/// @param _proof The merkle proof to prove the message.
/// @param _msgId The zero based index of the message in the inbox.
/// @param _to The address to send the message to.
/// @param _from The address of the message sender.
/// @param _message The message to relay.
function sendMessage(bytes32[] calldata _proof, uint64 _msgId, address _to, bytes calldata _message) external;
function sendMessage(
bytes32[] calldata _proof,
uint64 _msgId,
address _to,
address _from,
bytes calldata _message
) external;

/// @dev Resolves any challenge of the optimistic claim for 'epoch' using the canonical bridge.
/// Note: Access restricted to canonical bridge.
/// @param _epoch The epoch to verify.
/// @param _stateRoot The true state root for the epoch.
function resolveDisputedClaim(uint256 _epoch, bytes32 _stateRoot) external;

/// @dev Sets the allowlist for the sender gateway.
/// Note: Address(0) is used to allow all addresses.
/// @param _from The address to allow or disallow
/// @param _allow Whether to allow or disallow the address.
function setAllowlist(address _from, bool _allow) external;
}
5 changes: 4 additions & 1 deletion contracts/src/test/gateways/IReceiverGatewayMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,8 @@ import "../../interfaces/gateways/IReceiverGateway.sol";

interface IReceiverGatewayMock is IReceiverGateway {
/// Receive the message from the sender gateway.
function receiveMessage(address msgSender, uint256 _data) external;
function receiveMessage(uint256 _data) external;

/// Receive the message array from the sender gateway.
function receiveMessageArray(uint256[] calldata _data) external;
}
Loading
Loading