Skip to content

Commit

Permalink
feat(ctb): Pass msg.sender to dispute games from the factory (ether…
Browse files Browse the repository at this point in the history
…eum-optimism#10149)

* pass msg sender

* semver

bindings + semver

* bindings, locks, etc.

* bindings, locks, etc.
  • Loading branch information
clabby authored Apr 15, 2024
1 parent 2d354ac commit 4a157b4
Show file tree
Hide file tree
Showing 13 changed files with 135 additions and 46 deletions.
2 changes: 1 addition & 1 deletion op-bindings/bindings/disputegamefactory.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/disputegamefactory_more.go

Large diffs are not rendered by default.

35 changes: 33 additions & 2 deletions op-bindings/bindings/faultdisputegame.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/faultdisputegame_more.go

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions packages/contracts-bedrock/semver-lock.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,12 @@
"sourceCodeHash": "0xba941ad1f941f5a4a066182d50634fa9b190085ed82779decef71c019ba963c5"
},
"src/dispute/DisputeGameFactory.sol": {
"initCodeHash": "0xf4b8bc7cfaa1e741bc6bf1f9c8e5e20e3f77d603dd6f7f58af9fea7ebf0b070a",
"sourceCodeHash": "0x8545910bdb40f5e706a0ae5ed274cabc6d1f17be92d497a490d5732d74ac9c59"
"initCodeHash": "0xcbf28ecd117cbcbb9cea34dd75d2d0afbdc42865311b6fa94f23958c26186ab3",
"sourceCodeHash": "0x08f34cec56d58ea6ee7a47b5adcbeca6a68a5dd1daa949330b4bde86c2e605f5"
},
"src/dispute/FaultDisputeGame.sol": {
"initCodeHash": "0x4ea53ec8274b7a25012aab6655cd84a60b4cbcdba95ad199085fd81910731bee",
"sourceCodeHash": "0x778aafed19b2d8dddd61a44d39e94fb3139a89e416a314572534064ab8823ee1"
"initCodeHash": "0x98544bd91d1d777bfcfa0d71b81a9ed75f9feb31536aad934b92dcf16bb21ce7",
"sourceCodeHash": "0x06c7f5206e9c121c5c7fa2e6e114f9a80781025954c830bc7641f3ab0a5b2583"
},
"src/dispute/weth/DelayedWETH.sol": {
"initCodeHash": "0x7b6ec89eaec09e369426e73161a9c6932223bb1f974377190c3f6f552995da35",
Expand Down
13 changes: 13 additions & 0 deletions packages/contracts-bedrock/snapshots/abi/FaultDisputeGame.json
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,19 @@
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [],
"name": "gameCreator",
"outputs": [
{
"internalType": "address",
"name": "creator_",
"type": "address"
}
],
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [],
"name": "gameData",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,19 @@
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [],
"name": "gameCreator",
"outputs": [
{
"internalType": "address",
"name": "creator_",
"type": "address"
}
],
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [],
"name": "gameData",
Expand Down
16 changes: 13 additions & 3 deletions packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ contract DisputeGameFactory is OwnableUpgradeable, IDisputeGameFactory, ISemver
using LibClone for address;

/// @notice Semantic version.
/// @custom:semver 0.4.0
string public constant version = "0.4.0";
/// @custom:semver 0.5.0
string public constant version = "0.5.0";

/// @inheritdoc IDisputeGameFactory
mapping(GameType => IDisputeGame) public gameImpls;
Expand Down Expand Up @@ -103,7 +103,17 @@ contract DisputeGameFactory is OwnableUpgradeable, IDisputeGameFactory, ISemver
bytes32 parentHash = blockhash(block.number - 1);

// Clone the implementation contract and initialize it with the given parameters.
proxy_ = IDisputeGame(address(impl).clone(abi.encodePacked(_rootClaim, parentHash, _extraData)));
//
// CWIA Calldata Layout:
// ┌──────────────┬────────────────────────────────────┐
// │ Bytes │ Description │
// ├──────────────┼────────────────────────────────────┤
// │ [0, 20) │ Game creator address │
// │ [20, 52) │ Root claim │
// │ [52, 84) │ Parent block hash at creation time │
// │ [84, 84 + n) │ Extra data (opaque) │
// └──────────────┴────────────────────────────────────┘
proxy_ = IDisputeGame(address(impl).clone(abi.encodePacked(msg.sender, _rootClaim, parentHash, _extraData)));
proxy_.initialize{ value: msg.value }();

// Compute the unique identifier for the dispute game.
Expand Down
51 changes: 32 additions & 19 deletions packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
OutputRoot public startingOutputRoot;

/// @notice Semantic version.
/// @custom:semver 0.11.0
string public constant version = "0.11.0";
/// @custom:semver 0.12.0
string public constant version = "0.12.0";

/// @param _gameType The type ID of the game.
/// @param _absolutePrestate The absolute prestate of the instruction trace.
Expand Down Expand Up @@ -345,14 +345,9 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
}
}

/// @inheritdoc IFaultDisputeGame
function l1Head() public pure returns (Hash l1Head_) {
l1Head_ = Hash.wrap(_getArgBytes32(0x20));
}

/// @inheritdoc IFaultDisputeGame
function l2BlockNumber() public pure returns (uint256 l2BlockNumber_) {
l2BlockNumber_ = _getArgUint256(0x40);
l2BlockNumber_ = _getArgUint256(0x54);
}

////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -451,16 +446,26 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
resolvedSubgames[_claimIndex] = true;
}

/// @inheritdoc IDisputeGame
function gameCreator() public pure returns (address creator_) {
creator_ = _getArgAddress(0x00);
}

/// @inheritdoc IDisputeGame
function rootClaim() public pure returns (Claim rootClaim_) {
rootClaim_ = Claim.wrap(_getArgBytes32(0x00));
rootClaim_ = Claim.wrap(_getArgBytes32(0x14));
}

/// @inheritdoc IDisputeGame
function l1Head() public pure returns (Hash l1Head_) {
l1Head_ = Hash.wrap(_getArgBytes32(0x34));
}

/// @inheritdoc IDisputeGame
function extraData() public pure returns (bytes memory extraData_) {
// The extra data starts at the second word within the cwia calldata and
// is 32 bytes long.
extraData_ = _getArgBytes(0x40, 0x20);
extraData_ = _getArgBytes(0x54, 0x20);
}

/// @inheritdoc IDisputeGame
Expand Down Expand Up @@ -513,15 +518,23 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
// configured starting block number.
if (l2BlockNumber() <= rootBlockNumber) revert UnexpectedRootClaim(rootClaim());

// Revert if the calldata size is too large, which signals that the `extraData` contains more than expected.
// This is to prevent adding extra bytes to the `extraData` that result in a different game UUID in the factory,
// but are not used by the game, which would allow for multiple dispute games for the same output proposal to
// be created.
// Expected length: 0x66 (0x04 selector + 0x20 root claim + 0x20 l1 head + 0x20 extraData + 0x02 CWIA bytes)
// Revert if the calldata size is not the expected length.
//
// This is to prevent adding extra or omitting bytes from to `extraData` that result in a different game UUID
// in the factory, but are not used by the game, which would allow for multiple dispute games for the same
// output proposal to be created.
//
// Expected length: 0x7A
// - 0x04 selector
// - 0x14 creator address
// - 0x20 root claim
// - 0x20 l1 head
// - 0x20 extraData
// - 0x02 CWIA bytes
assembly {
if gt(calldatasize(), 0x66) {
// Store the selector for `ExtraDataTooLong()` & revert
mstore(0x00, 0xc407e025)
if iszero(eq(calldatasize(), 0x7A)) {
// Store the selector for `BadExtraData()` & revert
mstore(0x00, 0x9824bdab)
revert(0x1C, 0x04)
}
}
Expand All @@ -531,7 +544,7 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
ClaimData({
parentIndex: type(uint32).max,
counteredBy: address(0),
claimant: tx.origin,
claimant: gameCreator(),
bond: uint128(msg.value),
claim: rootClaim(),
position: ROOT_POSITION,
Expand Down
14 changes: 12 additions & 2 deletions packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,23 @@ interface IDisputeGame is IInitializable {
/// @return gameType_ The type of proof system being used.
function gameType() external view returns (GameType gameType_);

/// @notice Getter for the root claim.
/// @notice Getter for the creator of the dispute game.
/// @dev `clones-with-immutable-args` argument #1
/// @return creator_ The creator of the dispute game.
function gameCreator() external pure returns (address creator_);

/// @notice Getter for the root claim.
/// @dev `clones-with-immutable-args` argument #2
/// @return rootClaim_ The root claim of the DisputeGame.
function rootClaim() external pure returns (Claim rootClaim_);

/// @notice Getter for the parent hash of the L1 block when the dispute game was created.
/// @dev `clones-with-immutable-args` argument #3
/// @return l1Head_ The parent hash of the L1 block when the dispute game was created.
function l1Head() external pure returns (Hash l1Head_);

/// @notice Getter for the extra data.
/// @dev `clones-with-immutable-args` argument #2
/// @dev `clones-with-immutable-args` argument #4
/// @return extraData_ Any extra data supplied to the dispute game contract by the creator.
function extraData() external pure returns (bytes memory extraData_);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,6 @@ interface IFaultDisputeGame is IDisputeGame {
/// @param _claimIndex The index of the subgame root claim to resolve.
function resolveClaim(uint256 _claimIndex) external payable;

/// @notice A block hash on the L1 that contains the disputed output root.
function l1Head() external view returns (Hash l1Head_);

/// @notice The l2BlockNumber of the disputed output root in the `L2OutputOracle`.
function l2BlockNumber() external view returns (uint256 l2BlockNumber_);

Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/src/libraries/DisputeErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ error NoCreditToClaim();
/// @notice Thrown when the transfer of credit to a recipient account reverts.
error BondTransferFailed();

/// @notice Thrown when the `extraData` passed to the CWIA proxy is too long for the `FaultDisputeGame`.
error ExtraDataTooLong();
/// @notice Thrown when the `extraData` passed to the CWIA proxy is of improper length, or contains invalid information.
error BadExtraData();

/// @notice Thrown when a defense against the root claim is attempted.
error CannotDefendRootClaim();
Expand Down
18 changes: 10 additions & 8 deletions packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -178,15 +178,17 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
assertEq(delayedWeth.balanceOf(address(gameProxy)), _value);
}

/// @dev Tests that the game cannot be initialized with extra data > 64 bytes long (root claim + l2 block number
/// concatenated)
function testFuzz_initialize_extraDataTooLong_reverts(uint256 _extraDataLen) public {
/// @dev Tests that the game cannot be initialized with extra data of the incorrect length (must be 32 bytes)
function testFuzz_initialize_badExtraData_reverts(uint256 _extraDataLen) public {
// The `DisputeGameFactory` will pack the root claim and the extra data into a single array, which is enforced
// to be at least 64 bytes long.
// We bound the upper end to 23.5KB to ensure that the minimal proxy never surpasses the contract size limit
// in this test, as CWIA proxies store the immutable args in their bytecode.
// [33 bytes, 23.5 KB]
_extraDataLen = bound(_extraDataLen, 33, 23_500);
// [0 bytes, 31 bytes] u [33 bytes, 23.5 KB]
_extraDataLen = bound(_extraDataLen, 0, 23_500);
if (_extraDataLen == 32) {
_extraDataLen++;
}
bytes memory _extraData = new bytes(_extraDataLen);

// Assign the first 32 bytes in `extraData` to a valid L2 block number passed the starting block.
Expand All @@ -196,7 +198,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
}

Claim claim = _dummyClaim();
vm.expectRevert(abi.encodeWithSelector(ExtraDataTooLong.selector));
vm.expectRevert(abi.encodeWithSelector(BadExtraData.selector));
gameProxy = FaultDisputeGame(payable(address(disputeGameFactory.create(GAME_TYPE, claim, _extraData))));
}

Expand All @@ -214,7 +216,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
) = gameProxy.claimData(0);
assertEq(parentIndex, type(uint32).max);
assertEq(counteredBy, address(0));
assertEq(claimant, DEFAULT_SENDER);
assertEq(claimant, address(this));
assertEq(bond, 0);
assertEq(claim.raw(), ROOT_CLAIM.raw());
assertEq(position.raw(), 1);
Expand Down Expand Up @@ -435,7 +437,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
// Assert correctness of the parent claim's data.
assertEq(parentIndex, type(uint32).max);
assertEq(counteredBy, address(0));
assertEq(claimant, DEFAULT_SENDER);
assertEq(claimant, address(this));
assertEq(bond, 0);
assertEq(claim.raw(), ROOT_CLAIM.raw());
assertEq(position.raw(), 1);
Expand Down

0 comments on commit 4a157b4

Please sign in to comment.