fix: update OPSuccinctFaultDisputeGame for better security

book/fault_proofs/fault_proof_architecture.md

@@ -231,6 +231,11 @@ Validates a proof for a proposal:
     - If challenged: prover receives the challenger's bond
     - If unchallenged: no reward but can have fast finality
 
+Proving will revert if:
+- Proof is not submitted before the deadline
+- Proof is not valid
+- If a prover tries to prove a game that has already been proven
+
 ### Resolution
 
 ```solidity
@@ -244,6 +249,8 @@ Resolves the game by:
 - Ensure that the deadline has passed, and if the proposal is `Unchallenged`, then set `DEFENDER_WON`.
 - Ensure that the deadline has passed, and if the proposal is `Challenged`, then set `CHALLENGER_WON`
 - Distributing bonds based on outcome
+    - Distribution result is stored in `mapping(address => uint256) public credit;`.
+    - Actual distribution is done when appropriate recipient calls `claimCredit()`.
     - If parent game is `CHALLENGER_WON`, then the proposer's bond is distributed to the challenger.
       But if there was no challenge for the current game, then the proposer's bond is burned.
 

contracts/src/fp/OPSuccinctFaultDisputeGame.sol

@@ -23,6 +23,7 @@ import {
     ClockTimeExceeded,
     GameNotInProgress,
     IncorrectBondAmount,
+    NoCreditToClaim,
     UnexpectedRootClaim
 } from "src/dispute/lib/Errors.sol";
 import "src/fp/lib/Errors.sol";
@@ -142,6 +143,9 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
     /// @notice The claim made by the proposer.
     ClaimData public claimData;
 
+    /// @notice Credited balances for participants.
+    mapping(address => uint256) public credit;
+
     /// @notice The starting output root of the game that is proven from in case of a challenge.
     /// @dev This should match the claim root of the parent game.
     OutputRoot public startingOutputRoot;
@@ -257,9 +261,6 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
             deadline: Timestamp.wrap(uint64(block.timestamp + MAX_CHALLENGE_DURATION.raw()))
         });
 
-        // Hold the bond in the contract.
-        payable(address(this)).transfer(msg.value);
-
         // Set the game as initialized.
         initialized = true;
 
@@ -311,9 +312,6 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
         // Update the clock to the current block timestamp, which marks the start of the challenge.
         claimData.deadline = Timestamp.wrap(uint64(block.timestamp + MAX_PROVE_DURATION.raw()));
 
-        // Hold the bond in the contract.
-        payable(address(this)).transfer(msg.value);
-
         emit Challenged(claimData.counteredBy);
 
         return claimData.status;
@@ -325,6 +323,9 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
         // INVARIANT: Cannot prove a game if the clock has timed out.
         if (uint64(block.timestamp) > claimData.deadline.raw()) revert ClockTimeExceeded();
 
+        // INVARIANT: Cannot prove a claim that has already been proven
+        if (claimData.prover != address(0)) revert AlreadyProven();
+
         // Decode the public values to check the claim root
         AggregationOutputs memory publicValues = AggregationOutputs({
             l1Head: Hash.unwrap(l1Head()),
@@ -381,8 +382,8 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
             status = GameStatus.CHALLENGER_WINS;
             resolvedAt = Timestamp.wrap(uint64(block.timestamp));
 
-            // Distribute the bond to the challenger
-            payable(claimData.counteredBy).transfer(address(this).balance);
+            // Record the challenger's reward
+            credit[claimData.counteredBy] += address(this).balance;
 
             emit Resolved(status);
 
@@ -396,8 +397,8 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
             status = GameStatus.DEFENDER_WINS;
             resolvedAt = Timestamp.wrap(uint64(block.timestamp));
 
-            // Distribute the bond back to the proposer
-            payable(gameCreator()).transfer(address(this).balance);
+            // Record the proposer's reward
+            credit[gameCreator()] += address(this).balance;
 
             emit Resolved(status);
         } else if (claimData.status == ProposalStatus.Challenged) {
@@ -406,36 +407,51 @@ contract OPSuccinctFaultDisputeGame is Clone, ISemver {
             status = GameStatus.CHALLENGER_WINS;
             resolvedAt = Timestamp.wrap(uint64(block.timestamp));
 
-            // Distribute the bond to the challenger
-            payable(claimData.counteredBy).transfer(address(this).balance);
+            // Record the challenger's reward
+            credit[claimData.counteredBy] += address(this).balance;
 
             emit Resolved(status);
         } else if (claimData.status == ProposalStatus.UnchallengedAndValidProofProvided) {
             claimData.status = ProposalStatus.Resolved;
             status = GameStatus.DEFENDER_WINS;
             resolvedAt = Timestamp.wrap(uint64(block.timestamp));
 
-            // Return the initial bond to the proposer
-            payable(gameCreator()).transfer(address(this).balance);
+            // Record the proposer's reward
+            credit[gameCreator()] += address(this).balance;
 
             emit Resolved(status);
         } else if (claimData.status == ProposalStatus.ChallengedAndValidProofProvided) {
             claimData.status = ProposalStatus.Resolved;
             status = GameStatus.DEFENDER_WINS;
             resolvedAt = Timestamp.wrap(uint64(block.timestamp));
 
-            // Distribute the proof reward to the prover
-            payable(claimData.prover).transfer(PROOF_REWARD);
+            // Record the proof reward for the prover
+            credit[claimData.prover] += PROOF_REWARD;
 
-            // Return the initial bond to the proposer
-            payable(gameCreator()).transfer(address(this).balance);
+            // Record the remaining balance (proposer's bond) for the proposer
+            credit[gameCreator()] += address(this).balance - PROOF_REWARD;
 
             emit Resolved(status);
         }
 
         return status;
     }
 
+    /// @notice Claim the credit belonging to the recipient address.
+    /// @param _recipient The owner and recipient of the credit.
+    function claimCredit(address _recipient) external {
+        // Remove the credit from the recipient prior to performing the external call.
+        uint256 recipientCredit = credit[_recipient];
+        credit[_recipient] = 0;
+
+        // Revert if the recipient has no credit to claim.
+        if (recipientCredit == 0) revert NoCreditToClaim();
+
+        // Transfer the credit to the recipient.
+        (bool success,) = _recipient.call{value: recipientCredit}(hex"");
+        if (!success) revert BondTransferFailed();
+    }
+
     /// @notice Getter for the game type.
     /// @dev The reference impl should be entirely different depending on the type (fault, validity)
     ///      i.e. The game type should indicate the security model.

contracts/src/fp/lib/Errors.sol

@@ -16,3 +16,6 @@ error InvalidParentGame();
 
 /// @notice Thrown when the parent game is not resolved.
 error ParentGameNotResolved();
+
+/// @notice Thrown when the claim has already been proven.
+error AlreadyProven();

contracts/test/fp/OPSuccinctFaultDisputeGame.t.sol

@@ -8,9 +8,13 @@ import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.s
 // Libraries
 import {Claim, Duration, GameStatus, GameType, Hash, Timestamp} from "src/dispute/lib/Types.sol";
 import {
-    ClockNotExpired, IncorrectBondAmount, AlreadyInitialized, UnexpectedRootClaim
+    ClockNotExpired,
+    IncorrectBondAmount,
+    AlreadyInitialized,
+    UnexpectedRootClaim,
+    NoCreditToClaim
 } from "src/dispute/lib/Errors.sol";
-import {ParentGameNotResolved, InvalidParentGame, ClaimAlreadyChallenged} from "src/fp/lib/Errors.sol";
+import {ParentGameNotResolved, InvalidParentGame, ClaimAlreadyChallenged, AlreadyProven} from "src/fp/lib/Errors.sol";
 import {AggregationOutputs} from "src/lib/Types.sol";
 
 // Contracts
@@ -116,6 +120,7 @@ contract OPSuccinctFaultDisputeGameTest is Test {
         (,,,,, Timestamp parentGameDeadline) = parentGame.claimData();
         vm.warp(parentGameDeadline.raw() + 1 seconds);
         parentGame.resolve();
+        parentGame.claimCredit(proposer);
 
         // Create the child game referencing parent index = 0
         // The child game is at index 1
@@ -204,6 +209,9 @@ contract OPSuccinctFaultDisputeGameTest is Test {
         // Now we can resolve successfully
         game.resolve();
 
+        // Proposer gets the bond back
+        game.claimCredit(proposer);
+
         // Check final state
         assertEq(uint8(game.status()), uint8(GameStatus.DEFENDER_WINS));
         // The contract should have paid back the proposer
@@ -231,6 +239,13 @@ contract OPSuccinctFaultDisputeGameTest is Test {
         // Now the proposal is UnchallengedAndValidProofProvided; we can resolve immediately
         game.resolve();
 
+        // Prover does not get any credit
+        vm.expectRevert(NoCreditToClaim.selector);
+        game.claimCredit(prover);
+
+        // Proposer gets the bond back
+        game.claimCredit(proposer);
+
         // Final status: DEFENDER_WINS
         assertEq(uint8(game.status()), uint8(GameStatus.DEFENDER_WINS));
         assertEq(address(game).balance, 0);
@@ -285,6 +300,13 @@ contract OPSuccinctFaultDisputeGameTest is Test {
 
         // Resolve
         game.resolve();
+
+        // Prover gets the proof reward
+        game.claimCredit(prover);
+
+        // Proposer gets the bond back
+        game.claimCredit(proposer);
+
         assertEq(uint8(game.status()), uint8(GameStatus.DEFENDER_WINS));
         assertEq(address(game).balance, 0);
 
@@ -316,6 +338,10 @@ contract OPSuccinctFaultDisputeGameTest is Test {
 
         // Now we can resolve, resulting in CHALLENGER_WINS
         game.resolve();
+
+        // Challenger gets the bond back and wins proposer's bond
+        game.claimCredit(challenger);
+
         assertEq(uint8(game.status()), uint8(GameStatus.CHALLENGER_WINS));
 
         // The challenger receives the entire 3 ether
@@ -448,16 +474,36 @@ contract OPSuccinctFaultDisputeGameTest is Test {
 
         // 4) The game resolves as CHALLENGER_WINS
         game.resolve();
+
+        // Challenger gets the bond back and wins proposer's bond
+        game.claimCredit(challenger);
+
         assertEq(uint8(game.status()), uint8(GameStatus.CHALLENGER_WINS));
 
         // 5) If we try to resolve the child game, it should be resolved as CHALLENGER_WINS
         // because parent's claim is invalid.
         // The child's bond is lost since there is no challenger for the child game.
         childGame.resolve();
+
+        // Challenger hasn't challenged the child game, so it gets nothing
+        vm.expectRevert(NoCreditToClaim.selector);
+        childGame.claimCredit(challenger);
+
         assertEq(uint8(childGame.status()), uint8(GameStatus.CHALLENGER_WINS));
 
-        assertEq(address(childGame).balance, 0);
+        assertEq(address(childGame).balance, 1 ether);
         assertEq(address(challenger).balance, 3 ether);
         assertEq(address(proposer).balance, 0 ether);
     }
+
+    // =========================================
+    // Test: Attempting multiple `prove()` calls
+    // =========================================
+    function testCannotProveMultipleTimes() public {
+        vm.startPrank(prover);
+        game.prove(bytes(""));
+        vm.expectRevert(AlreadyProven.selector);
+        game.prove(bytes(""));
+        vm.stopPrank();
+    }
 }