Skip to content
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

Apply 7702 patch #291

Merged
merged 16 commits into from
Feb 5, 2025
Merged
4 changes: 3 additions & 1 deletion audit-ci.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@
// Server-Side Request Forgery in axios
"GHSA-8hc4-vh64-cxmj",
// Regular Expression Denial of Service (ReDoS) in micromatch
"GHSA-952p-6rrq-rcjv"
"GHSA-952p-6rrq-rcjv",
// cookie accepts cookie name, path, and domain with out of bounds characters
"GHSA-pxg6-pf52-xh8x"
]
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@arbitrum/nitro-contracts",
"version": "2.1.2",
"version": "2.1.3",
"description": "Layer 2 precompiles and rollup for Arbitrum Nitro",
"author": "Offchain Labs, Inc.",
"license": "BUSL-1.1",
Expand Down
2 changes: 1 addition & 1 deletion slither.db.json

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions src/bridge/AbsInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ import {
InsufficientSubmissionCost,
L1Forked,
NotAllowedOrigin,
NotOrigin,
NotCodelessOrigin,
NotRollupOrOwner,
RetryableData
} from "../libraries/Error.sol";
import "./IInboxBase.sol";
import "./ISequencerInbox.sol";
import "./IBridge.sol";
import "../libraries/AddressAliasHelper.sol";
import "../libraries/CallerChecker.sol";
import "../libraries/DelegateCallAware.sol";
import {
L1MessageType_submitRetryableTx,
Expand Down Expand Up @@ -138,8 +139,7 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
returns (uint256)
{
if (_chainIdChanged()) revert L1Forked();
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
if (!CallerChecker.isCallerCodelessOrigin()) revert NotCodelessOrigin();
if (messageData.length > maxDataSize) revert DataTooLarge(messageData.length, maxDataSize);
uint256 msgNum = _deliverToBridge(L2_MSG, msg.sender, keccak256(messageData), 0);
emit InboxMessageDeliveredFromOrigin(msgNum);
Expand Down
3 changes: 3 additions & 0 deletions src/bridge/Inbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ contract Inbox is AbsInbox, IInbox {
if (!_chainIdChanged()) revert NotForked();
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
// no code size check required because we only want to know if msg.sender is an EOA to undo alias
// arbos will discard unsigned tx with gas limit too large
if (gasLimit > type(uint64).max) {
revert GasLimitTooLarge();
Expand Down Expand Up @@ -152,6 +153,7 @@ contract Inbox is AbsInbox, IInbox {
if (!_chainIdChanged()) revert NotForked();
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
// no code size check required because we only want to know if msg.sender is an EOA to undo alias
// arbos will discard unsigned tx with gas limit too large
if (gasLimit > type(uint64).max) {
revert GasLimitTooLarge();
Expand Down Expand Up @@ -185,6 +187,7 @@ contract Inbox is AbsInbox, IInbox {
if (!_chainIdChanged()) revert NotForked();
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
// no code size check required because we only want to know if msg.sender is an EOA to undo alias
// arbos will discard unsigned tx with gas limit too large
if (gasLimit > type(uint64).max) {
revert GasLimitTooLarge();
Expand Down
10 changes: 5 additions & 5 deletions src/bridge/SequencerInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
NoSuchKeyset,
NotForked,
NotBatchPosterManager,
NotCodelessOrigin,
RollupNotChanged,
DataBlobsNotSupported,
InitParamZero,
Expand All @@ -38,6 +39,7 @@ import "../rollup/IRollupLogic.sol";
import "./Messages.sol";
import "../precompiles/ArbGasInfo.sol";
import "../precompiles/ArbSys.sol";
import "../libraries/CallerChecker.sol";
import "../libraries/IReader4844.sol";

import {L1MessageType_batchPostingReport} from "../libraries/MessageTypes.sol";
Expand Down Expand Up @@ -360,8 +362,7 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
uint256 prevMessageCount,
uint256 newMessageCount
) external refundsGas(gasRefunder, IReader4844(address(0))) {
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
if (!CallerChecker.isCallerCodelessOrigin()) revert NotCodelessOrigin();
if (!isBatchPoster[msg.sender]) revert NotBatchPoster();
(bytes32 dataHash, IBridge.TimeBounds memory timeBounds) = formCallDataHash(
data,
Expand Down Expand Up @@ -457,10 +458,9 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
if (hostChainIsArbitrum) revert DataBlobsNotSupported();

// submit a batch spending report to refund the entity that produced the blob batch data
// same as using calldata, we only submit spending report if the caller is the origin of the tx
// same as using calldata, we only submit spending report if the caller is the origin and is codeless
// such that one cannot "double-claim" batch posting refund in the same tx
// solhint-disable-next-line avoid-tx-origin
if (msg.sender == tx.origin && !isUsingFeeToken) {
if (CallerChecker.isCallerCodelessOrigin() && !isUsingFeeToken) {
submitBatchSpendingReport(dataHash, seqMessageIndex, block.basefee, blobGas);
}
}
Expand Down
18 changes: 18 additions & 0 deletions src/libraries/CallerChecker.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2021-2024, Offchain Labs, Inc.
// For license information, see https://github.com/OffchainLabs/nitro-contracts/blob/main/LICENSE
// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.0;

library CallerChecker {
/**
* @notice A EIP-7702 safe check to ensure the caller is the origin and is codeless
* @return bool true if the caller is the origin and is codeless, false otherwise
* @dev If the caller is the origin and is codeless, then msg.data is guaranteed to be same as tx.data
* It also mean the caller would not be able to call a contract multiple times with the same transaction
*/
function isCallerCodelessOrigin() internal view returns (bool) {
// solhint-disable-next-line avoid-tx-origin
return msg.sender == tx.origin && msg.sender.code.length == 0;
}
}
3 changes: 3 additions & 0 deletions src/libraries/Error.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ error HadZeroInit();
/// @dev Thrown when post upgrade init validation fails
error BadPostUpgradeInit();

/// @dev Thrown when the caller is not a codeless origin
error NotCodelessOrigin();

/// @dev Thrown when non owner tries to access an only-owner function
/// @param sender The msg.sender who is not the owner
/// @param owner The owner address
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/GasRefundEnabled.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pragma solidity ^0.8.0;

import "./IReader4844.sol";
import "./IGasRefunder.sol";
import "../libraries/CallerChecker.sol";

abstract contract GasRefundEnabled {
uint256 internal immutable gasPerBlob = 2**17;
Expand All @@ -24,8 +25,7 @@ abstract contract GasRefundEnabled {
startGasLeft += calldataWords * 6 + (calldataWords**2) / 512;
// if triggered in a contract call, the spender may be overrefunded by appending dummy data to the call
// so we check if it is a top level call, which would mean the sender paid calldata as part of tx.input
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) {
if (!CallerChecker.isCallerCodelessOrigin()) {
// We can't be sure if this calldata came from the top level tx,
// so to be safe we tell the gas refunder there was no calldata.
calldataSize = 0;
Expand Down
11 changes: 10 additions & 1 deletion test/foundry/AbsInbox.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,19 @@ abstract contract AbsInboxTest is Test {
}

function test_sendL2MessageFromOrigin_revert_NotOrigin() public {
vm.expectRevert(abi.encodeWithSelector(NotOrigin.selector));
vm.expectRevert(abi.encodeWithSelector(NotCodelessOrigin.selector));
inbox.sendL2MessageFromOrigin(abi.encodePacked("some msg"));
}

function test_sendL2MessageFromOrigin_revert_NotCodeless() public {
assertEq(user.code.length, 0, "user is codeless");
vm.etch(user, bytes("some code"));
vm.prank(user, user);
vm.expectRevert(abi.encodeWithSelector(NotCodelessOrigin.selector));
inbox.sendL2MessageFromOrigin(abi.encodePacked("some msg"));
vm.etch(user, bytes(""));
}

function test_sendL2Message() public {
// L2 msg params
bytes memory data = abi.encodePacked("some msg");
Expand Down
16 changes: 15 additions & 1 deletion test/foundry/SequencerInbox.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ contract SequencerInboxTest is Test {
uint256 sequenceNumber = bridge.sequencerMessageCount();
uint256 delayedMessagesRead = bridge.delayedMessageCount();

vm.expectRevert(abi.encodeWithSelector(NotOrigin.selector));
vm.expectRevert(abi.encodeWithSelector(NotCodelessOrigin.selector));
seqInbox.addSequencerL2BatchFromOrigin(
sequenceNumber,
data,
Expand All @@ -419,6 +419,20 @@ contract SequencerInboxTest is Test {
subMessageCount + 1
);

assertEq(rollupOwner.code.length, 0, "rollupOwner is codeless");
vm.etch(rollupOwner, bytes("some code"));
vm.prank(rollupOwner, rollupOwner);
vm.expectRevert(abi.encodeWithSelector(NotCodelessOrigin.selector));
seqInbox.addSequencerL2BatchFromOrigin(
sequenceNumber,
data,
delayedMessagesRead,
IGasRefunder(address(0)),
subMessageCount,
subMessageCount + 1
);
vm.etch(rollupOwner, bytes(""));

vm.prank(rollupOwner);
seqInbox.setIsBatchPoster(tx.origin, false);

Expand Down
Loading