fix: correct clock extension for step (#258)

When the FDG is about to execute a step, we must add an additional
clock extension equal to the PreimageOracle challenge period. This
gives the PreimageOracle time to elapse.

packages/contracts-bedrock/semver-lock.json

@@ -148,8 +148,8 @@
     "sourceCodeHash": "0x115bd6a4c4d77ed210dfd468675b409fdae9f79b932063c138f0765ba9063462"
   },
   "src/cannon/PreimageOracle.sol": {
-    "initCodeHash": "0x3b89440c73c7da079257f38a9da237fef199755c0e31578889ebd55e3f963aef",
-    "sourceCodeHash": "0x6889891526f82ec0c68561ac0803da0ee995f737844ad2b7acfdac0b41ff789e"
+    "initCodeHash": "0x967d730bb1b10a36ee625179734cccd6b6826ce480bad0419272663c460603bd",
+    "sourceCodeHash": "0xb1f0d6f26c2e6a2c3b635eaf8f327e91a8d22ef7479b1ebb93427b88f73ed163"
   },
   "src/dispute/AnchorStateRegistry.sol": {
     "initCodeHash": "0x0305c21e50829b9e07d43358d8c2c82f1449534c90d4391400d46e76d0503a49",
@@ -160,8 +160,8 @@
     "sourceCodeHash": "0x918c395ac5d77357f2551616aad0613e68893862edd14e554623eb16ee6ba148"
   },
   "src/dispute/FaultDisputeGame.sol": {
-    "initCodeHash": "0x8a5b0a63aa89d54bfdfee0520c7d93d148dcf1bfaefd0649d070492cf1b40327",
-    "sourceCodeHash": "0x8412814c752106251edf5d96881c8e00b1962b406ef2920837e9b4a1209a408b"
+    "initCodeHash": "0xe0b6dae6078d1e7237240ab3c589055190ff4a4bb16280dcbf3919e062ad8f84",
+    "sourceCodeHash": "0xea69f63d4df701e9dfb73b85692c9a62b8a5819b3f473889c9da3734131b9738"
   },
   "src/dispute/weth/DelayedWETH.sol": {
     "initCodeHash": "0x8f9a5b50374331ad2fabe03a7ce28a0012bfaca5fa48ee917339c3eec39a319f",

packages/contracts-bedrock/snapshots/abi/FaultDisputeGame.json

@@ -963,6 +963,11 @@
     "name": "IncorrectBondAmount",
     "type": "error"
   },
+  {
+    "inputs": [],
+    "name": "InvalidChallengePeriod",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "InvalidClockExtension",

packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGame.json

@@ -1004,6 +1004,11 @@
     "name": "IncorrectBondAmount",
     "type": "error"
   },
+  {
+    "inputs": [],
+    "name": "InvalidChallengePeriod",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "InvalidClockExtension",

packages/contracts-bedrock/src/cannon/PreimageOracle.sol

@@ -92,6 +92,11 @@ contract PreimageOracle is IPreimageOracle, ISemver {
         MIN_LPP_SIZE_BYTES = _minProposalSize;
         CHALLENGE_PERIOD = _challengePeriod;
 
+        // Make sure challenge period fits within uint64 so that it can safely be used within the
+        // FaultDisputeGame contract to compute clock extensions. Adding this check is simpler than
+        // changing the existing contract ABI.
+        require(_challengePeriod <= type(uint64).max, "challenge period too large");
+
         // Compute hashes in empty sparse Merkle tree. The first hash is not set, and kept as zero as the identity.
         for (uint256 height = 0; height < KECCAK_TREE_DEPTH - 1; height++) {
             zeroHashes[height + 1] = keccak256(abi.encodePacked(zeroHashes[height], zeroHashes[height]));

packages/contracts-bedrock/src/cannon/interfaces/IPreimageOracle.sol

@@ -4,6 +4,10 @@ pragma solidity 0.8.15;
 /// @title IPreimageOracle
 /// @notice Interface for a preimage oracle.
 interface IPreimageOracle {
+    /// @notice Returns the length of the large preimage proposal challenge period.
+    /// @return challengePeriod_ The length of the challenge period in seconds.
+    function challengePeriod() external view returns (uint256 challengePeriod_);
+
     /// @notice Reads a preimage from the oracle.
     /// @param _key The key of the preimage to read.
     /// @param _offset The offset of the preimage to read.

packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol

@@ -2,6 +2,7 @@
 pragma solidity 0.8.15;
 
 import { FixedPointMathLib } from "@solady/utils/FixedPointMathLib.sol";
+import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
 
 import { IDelayedWETH } from "src/dispute/interfaces/IDelayedWETH.sol";
 import { IDisputeGame } from "src/dispute/interfaces/IDisputeGame.sol";
@@ -146,11 +147,23 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
         // The split depth cannot be 0 or 1 to stay in bounds of clock extension arithmetic.
         if (_splitDepth < 2) revert InvalidSplitDepth();
 
-        // The clock extension may not be greater than the max clock duration.
-        if (_clockExtension.raw() > _maxClockDuration.raw()) revert InvalidClockExtension();
+        // The PreimageOracle challenge period must fit into uint64 so we can safely use it here.
+        // Runtime check was added instead of changing the ABI since the contract is already
+        // deployed in production. We perform the same check within the PreimageOracle for the
+        // benefit of developers but also perform this check here defensively.
+        if (_vm.oracle().challengePeriod() > type(uint64).max) revert InvalidChallengePeriod();
 
-        // The worst-case clock extension may not be greater than the max clock duration.
-        if (_clockExtension.raw() * 2 > _maxClockDuration.raw()) revert InvalidClockExtension();
+        // Determine the maximum clock extension which is either the split depth extension or the
+        // maximum game depth extension depending on the configuration of these contracts.
+        uint256 splitDepthExtension = uint256(_clockExtension.raw()) * 2;
+        uint256 maxGameDepthExtension = uint256(_clockExtension.raw()) + uint256(_vm.oracle().challengePeriod());
+        uint256 maxClockExtension = Math.max(splitDepthExtension, maxGameDepthExtension);
+
+        // The maximum clock extension must fit into a uint64.
+        if (maxClockExtension > type(uint64).max) revert InvalidClockExtension();
+
+        // The maximum clock extension may not be greater than the maximum clock duration.
+        if (uint64(maxClockExtension) > _maxClockDuration.raw()) revert InvalidClockExtension();
 
         // Set up initial game state.
         GAME_TYPE = _gameType;
@@ -380,17 +393,29 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver {
         //            seconds of time.
         if (nextDuration.raw() == MAX_CLOCK_DURATION.raw()) revert ClockTimeExceeded();
 
-        // If the remaining clock time has less than `CLOCK_EXTENSION` seconds remaining, grant the potential
-        // grandchild's clock `CLOCK_EXTENSION` seconds. This is to ensure that, even if a player has to inherit another
-        // team's clock to counter a freeloader claim, they will always have enough time to respond. This extension
-        // is bounded by the depth of the tree. If the potential grandchild is an execution trace bisection root, the
-        // clock extension is doubled. This is to allow for extra time for the off-chain challenge agent to generate
-        // the initial instruction trace on the native FPVM.
-        if (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw()) {
-            // If the potential grandchild is an execution trace bisection root, double the clock extension.
-            uint64 extensionPeriod =
-                nextPositionDepth == SPLIT_DEPTH - 1 ? CLOCK_EXTENSION.raw() * 2 : CLOCK_EXTENSION.raw();
-            nextDuration = Duration.wrap(MAX_CLOCK_DURATION.raw() - extensionPeriod);
+        // Clock extension is a mechanism that automatically extends the clock for a potential
+        // grandchild claim when there would be less than the clock extension time left if a player
+        // is forced to inherit another team's clock when countering a freeloader claim. Exact
+        // amount of clock extension time depends exactly where we are within the game.
+        uint64 actualExtension;
+        if (nextPositionDepth == MAX_GAME_DEPTH - 1) {
+            // If the next position is `MAX_GAME_DEPTH - 1` then we're about to execute a step. Our
+            // clock extension must therefore account for the LPP challenge period in addition to
+            // the standard clock extension.
+            actualExtension = CLOCK_EXTENSION.raw() + uint64(VM.oracle().challengePeriod());
+        } else if (nextPositionDepth == SPLIT_DEPTH - 1) {
+            // If the next position is `SPLIT_DEPTH - 1` then we're about to begin an execution
+            // trace bisection and we need to give extra time for the off-chain challenge agent to
+            // be able to generate the initial instruction trace on the native FPVM.
+            actualExtension = CLOCK_EXTENSION.raw() * 2;
+        } else {
+            // Otherwise, we just use the standard clock extension.
+            actualExtension = CLOCK_EXTENSION.raw();
+        }
+
+        // Check if we need to apply the clock extension.
+        if (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - actualExtension) {
+            nextDuration = Duration.wrap(MAX_CLOCK_DURATION.raw() - actualExtension);
         }
 
         // Construct the next clock with the new duration and the current block timestamp.

packages/contracts-bedrock/src/dispute/lib/Errors.sol

@@ -94,6 +94,9 @@ error InvalidSplitDepth();
 /// @notice Thrown on deployment if the max clock duration is less than or equal to the clock extension.
 error InvalidClockExtension();
 
+/// @notice Thrown on deployment if the PreimageOracle challenge period is too high.
+error InvalidChallengePeriod();
+
 /// @notice Thrown on deployment if the max depth is greater than `LibPosition.`
 error MaxDepthTooLarge();
 

packages/contracts-bedrock/test/cannon/PreimageOracle.t.sol

@@ -20,6 +20,14 @@ contract PreimageOracle_Test is Test {
         vm.label(address(oracle), "PreimageOracle");
     }
 
+    /// @notice Tests that the challenge period cannot be made too large.
+    /// @param _challengePeriod The challenge period to test.
+    function testFuzz_constructor_challengePeriodTooLarge_reverts(uint256 _challengePeriod) public {
+        _challengePeriod = bound(_challengePeriod, uint256(type(uint64).max) + 1, type(uint256).max);
+        vm.expectRevert("challenge period too large");
+        new PreimageOracle(0, _challengePeriod);
+    }
+
     /// @notice Test the pre-image key computation with a known pre-image.
     function test_keccak256PreimageKey_succeeds() public pure {
         bytes memory preimage = hex"deadbeef";

packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol

@@ -45,7 +45,10 @@ contract FaultDisputeGame_Init is DisputeGameFactory_Init {
         // Set the extra data for the game creation
         extraData = abi.encode(l2BlockNumber);
 
-        AlphabetVM _vm = new AlphabetVM(absolutePrestate, new PreimageOracle(0, 0));
+        // Set preimage oracle challenge period to something arbitrary (4 seconds) just so we can
+        // actually test the clock extensions later on. This is not a realistic value.
+        PreimageOracle oracle = new PreimageOracle(0, 4);
+        AlphabetVM _vm = new AlphabetVM(absolutePrestate, oracle);
 
         // Deploy an implementation of the fault game
         gameImpl = new FaultDisputeGame({
@@ -128,6 +131,37 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
         });
     }
 
+    /// @dev Tests that the constructor of the `FaultDisputeGame` reverts when the challenge period
+    //       of the preimage oracle being used by the game's VM is too large.
+    /// @param _challengePeriod The challenge period of the preimage oracle.
+    function testFuzz_constructor_oracleChallengePeriodTooLarge_reverts(uint256 _challengePeriod) public {
+        _challengePeriod = bound(_challengePeriod, uint256(type(uint64).max) + 1, type(uint256).max);
+
+        PreimageOracle oracle = new PreimageOracle(0, 0);
+        AlphabetVM alphabetVM = new AlphabetVM(absolutePrestate, oracle);
+
+        // PreimageOracle constructor will revert if the challenge period is too large, so we need
+        // to mock the call to pretend this is a bugged implementation where the challenge period
+        // is allowed to be too large.
+        vm.mockCall(
+            address(oracle), abi.encodeWithSelector(oracle.challengePeriod.selector), abi.encode(_challengePeriod)
+        );
+
+        vm.expectRevert(InvalidChallengePeriod.selector);
+        new FaultDisputeGame({
+            _gameType: GAME_TYPE,
+            _absolutePrestate: absolutePrestate,
+            _maxGameDepth: 2 ** 3,
+            _splitDepth: 2 ** 2,
+            _clockExtension: Duration.wrap(3 hours),
+            _maxClockDuration: Duration.wrap(3.5 days),
+            _vm: alphabetVM,
+            _weth: DelayedWETH(payable(address(0))),
+            _anchorStateRegistry: IAnchorStateRegistry(address(0)),
+            _l2ChainId: 10
+        });
+    }
+
     /// @dev Tests that the constructor of the `FaultDisputeGame` reverts when the `_splitDepth`
     ///      parameter is greater than or equal to the `MAX_GAME_DEPTH`
     function testFuzz_constructor_invalidSplitDepth_reverts(uint256 _splitDepth) public {
@@ -482,47 +516,126 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
         assertEq(clock.raw(), LibClock.wrap(Duration.wrap(20), Timestamp.wrap(uint64(block.timestamp))).raw());
     }
 
-    /// @notice Static unit test that checks proper clock extension.
-    function test_move_clockExtensionCorrectness_succeeds() public {
+    /// @dev Tests that the standard clock extension is triggered for a move that is not the
+    ///      split depth or the max game depth.
+    function test_move_standardClockExtension_succeeds() public {
         (,,,,,, Clock clock) = gameProxy.claimData(0);
         assertEq(clock.raw(), LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp))).raw());
 
+        uint256 bond;
+        Claim disputed;
         Claim claim = _dummyClaim();
         uint256 splitDepth = gameProxy.splitDepth();
         uint64 halfGameDuration = gameProxy.maxClockDuration().raw();
         uint64 clockExtension = gameProxy.clockExtension().raw();
 
-        // Make an initial attack against the root claim with 1 second left on the clock. The grandchild should be
-        // allocated exactly `clockExtension` seconds remaining on their potential clock.
-        vm.warp(block.timestamp + halfGameDuration - 1 seconds);
-        uint256 bond = _getRequiredBond(0);
-        (,,,, Claim disputed,,) = gameProxy.claimData(0);
+        // Warp ahead so that the next move will trigger a clock extension. We warp to the very
+        // first timestamp where a clock extension should be triggered.
+        vm.warp(block.timestamp + halfGameDuration - clockExtension + 1 seconds);
+
+        // Execute a move that should cause a clock extension.
+        bond = _getRequiredBond(0);
+        (,,,, disputed,,) = gameProxy.claimData(0);
         gameProxy.attack{ value: bond }(disputed, 0, claim);
         (,,,,,, clock) = gameProxy.claimData(1);
+
+        // The clock should have been pushed back to the clock extension time.
         assertEq(clock.duration().raw(), halfGameDuration - clockExtension);
 
-        // Warp ahead to the last second of the root claim defender's clock, and bisect all the way down to the move
-        // above the `SPLIT_DEPTH`. This warp guarantees that all moves from here on out will have clock extensions.
-        vm.warp(block.timestamp + halfGameDuration - 1 seconds);
+        // Warp ahead again so that clock extensions will also trigger for the other team. Here we
+        // only warp to the clockExtension time because we'll be warping ahead by one second during
+        // each additional move.
+        vm.warp(block.timestamp + halfGameDuration - clockExtension);
+
+        // Work our way down to the split depth.
         for (uint256 i = 1; i < splitDepth - 2; i++) {
+            // Warp ahead by one second so that the next move will trigger a clock extension.
+            vm.warp(block.timestamp + 1 seconds);
+
+            // Execute a move that should cause a clock extension.
             bond = _getRequiredBond(i);
             (,,,, disputed,,) = gameProxy.claimData(i);
             gameProxy.attack{ value: bond }(disputed, i, claim);
+            (,,,,,, clock) = gameProxy.claimData(i + 1);
+
+            // The clock should have been pushed back to the clock extension time.
+            assertEq(clock.duration().raw(), halfGameDuration - clockExtension);
         }
+    }
 
-        // Warp ahead 1 seconds to have `clockExtension - 1 seconds` left on the next move's clock.
-        vm.warp(block.timestamp + 1 seconds);
+    function test_move_splitDepthClockExtension_succeeds() public {
+        (,,,,,, Clock clock) = gameProxy.claimData(0);
+        assertEq(clock.raw(), LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp))).raw());
+
+        uint256 bond;
+        Claim disputed;
+        Claim claim = _dummyClaim();
+        uint256 splitDepth = gameProxy.splitDepth();
+        uint64 halfGameDuration = gameProxy.maxClockDuration().raw();
+        uint64 clockExtension = gameProxy.clockExtension().raw();
+
+        // Work our way down to the split depth without moving ahead in time, we don't care about
+        // the exact clock here, just don't want take the clock below the clock extension time that
+        // we're trying to test here.
+        for (uint256 i = 0; i < splitDepth - 2; i++) {
+            bond = _getRequiredBond(i);
+            (,,,, disputed,,) = gameProxy.claimData(i);
+            gameProxy.attack{ value: bond }(disputed, i, claim);
+        }
+
+        // Warp ahead to the very first timestamp where a clock extension should be triggered.
+        vm.warp(block.timestamp + halfGameDuration - clockExtension * 2 + 1 seconds);
 
-        // The move above the split depth's grand child is the execution trace bisection root. The grandchild should
-        // be allocated `clockExtension * 2` seconds on their potential clock, if currently they have less than
-        // `clockExtension` seconds left.
+        // Execute a move that should cause a clock extension.
         bond = _getRequiredBond(splitDepth - 2);
         (,,,, disputed,,) = gameProxy.claimData(splitDepth - 2);
         gameProxy.attack{ value: bond }(disputed, splitDepth - 2, claim);
         (,,,,,, clock) = gameProxy.claimData(splitDepth - 1);
+
+        // The clock should have been pushed back to the clock extension time.
         assertEq(clock.duration().raw(), halfGameDuration - clockExtension * 2);
     }
 
+    function test_move_maxGameDepthClockExtension_succeeds() public {
+        (,,,,,, Clock clock) = gameProxy.claimData(0);
+        assertEq(clock.raw(), LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp))).raw());
+
+        uint256 bond;
+        Claim disputed;
+        Claim claim = _dummyClaim();
+        uint256 splitDepth = gameProxy.splitDepth();
+        uint64 halfGameDuration = gameProxy.maxClockDuration().raw();
+        uint64 clockExtension = gameProxy.clockExtension().raw();
+
+        // Work our way down to the split depth without moving ahead in time, we don't care about
+        // the exact clock here, just don't want take the clock below the clock extension time that
+        // we're trying to test here.
+        for (uint256 i = 0; i < gameProxy.maxGameDepth() - 2; i++) {
+            bond = _getRequiredBond(i);
+            (,,,, disputed,,) = gameProxy.claimData(i);
+            gameProxy.attack{ value: bond }(disputed, i, claim);
+
+            // Change the claim status when we're crossing the split depth.
+            if (i == splitDepth - 2) {
+                claim = _changeClaimStatus(claim, VMStatuses.PANIC);
+            }
+        }
+
+        // Warp ahead to the very first timestamp where a clock extension should be triggered.
+        vm.warp(block.timestamp + halfGameDuration - (clockExtension + gameProxy.vm().oracle().challengePeriod()) + 1);
+
+        // Execute a move that should cause a clock extension.
+        bond = _getRequiredBond(gameProxy.maxGameDepth() - 2);
+        (,,,, disputed,,) = gameProxy.claimData(gameProxy.maxGameDepth() - 2);
+        gameProxy.attack{ value: bond }(disputed, gameProxy.maxGameDepth() - 2, claim);
+        (,,,,,, clock) = gameProxy.claimData(gameProxy.maxGameDepth() - 1);
+
+        // The clock should have been pushed back to the clock extension time.
+        assertEq(
+            clock.duration().raw(), halfGameDuration - (clockExtension + gameProxy.vm().oracle().challengePeriod())
+        );
+    }
+
     /// @dev Tests that an identical claim cannot be made twice. The duplicate claim attempt should
     ///      revert with the `ClaimAlreadyExists` error.
     function test_move_duplicateClaim_reverts() public {