fix: malicious arbitrable M-01

unknownunknown1 · jaybuidl · commit b8af0851bd02 · 2025-12-19T18:26:06.000Z
diff --git a/contracts/src/arbitration/arbitrables/ArbitrableExample.sol b/contracts/src/arbitration/arbitrables/ArbitrableExample.sol
@@ -149,7 +149,7 @@ contract ArbitrableExample is IArbitrableV2 {
     }
 
     /// @inheritdoc IArbitrableV2
-    function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override {
+    function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external virtual override {
         uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
         DisputeStruct storage dispute = disputes[localDisputeID];
         if (msg.sender != address(arbitrator)) revert ArbitratorOnly();
diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol b/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
@@ -475,8 +475,8 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
         address payable _beneficiary,
         uint256 _choice
     ) external returns (uint256 amount) {
-        (, , , bool isRuled, ) = core.disputes(_coreDisputeID);
-        if (!isRuled) revert DisputeNotResolved();
+        (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID);
+        if (period != KlerosCore.Period.execution) revert DisputeNotResolved();
         if (core.paused()) revert CoreIsPaused();
         if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit();
 
diff --git a/contracts/src/test/MaliciousArbitrableMock.sol b/contracts/src/test/MaliciousArbitrableMock.sol
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.24;
+
+import {IArbitratorV2, IDisputeTemplateRegistry, IERC20, ArbitrableExample} from "../arbitration/arbitrables/ArbitrableExample.sol";
+
+/// @title MaliciousArbitrableMock
+/// A mock contract to check intentional rule() revert.
+contract MaliciousArbitrableMock is ArbitrableExample {
+    bool public doRevert;
+
+    function changeBehaviour(bool _doRevert) external {
+        doRevert = _doRevert;
+    }
+
+    constructor(
+        IArbitratorV2 _arbitrator,
+        string memory _templateData,
+        string memory _templateDataMappings,
+        bytes memory _arbitratorExtraData,
+        IDisputeTemplateRegistry _templateRegistry,
+        IERC20 _weth
+    )
+        ArbitrableExample(
+            _arbitrator,
+            _templateData,
+            _templateDataMappings,
+            _arbitratorExtraData,
+            _templateRegistry,
+            _weth
+        )
+    {
+        doRevert = true;
+    }
+
+    function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override {
+        if (doRevert) revert RuleReverted();
+
+        uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
+        DisputeStruct storage dispute = disputes[localDisputeID];
+        if (msg.sender != address(arbitrator)) revert ArbitratorOnly();
+        if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds();
+        if (dispute.isRuled) revert DisputeAlreadyRuled();
+
+        dispute.isRuled = true;
+        dispute.ruling = _ruling;
+
+        emit Ruling(IArbitratorV2(msg.sender), _arbitratorDisputeID, dispute.ruling);
+    }
+
+    error RuleReverted();
+}
diff --git a/contracts/test/foundry/KlerosCore_Execution.t.sol b/contracts/test/foundry/KlerosCore_Execution.t.sol
@@ -9,6 +9,7 @@ import {IArbitratorV2, IArbitrableV2} from "../../src/arbitration/KlerosCore.sol
 import {IERC20} from "../../src/libraries/SafeERC20.sol";
 import "../../src/libraries/Constants.sol";
 import {console} from "forge-std/console.sol";
+import {MaliciousArbitrableMock} from "../../src/test/MaliciousArbitrableMock.sol";
 
 /// @title KlerosCore_ExecutionTest
 /// @dev Tests for KlerosCore execution, rewards, and ruling finalization
@@ -655,6 +656,48 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
         assertEq(ruled, true, "Should be ruled");
     }
 
+    function test_executeRuling_arbitrableRevert() public {
+        MaliciousArbitrableMock maliciousArbitrable = new MaliciousArbitrableMock(
+            core,
+            templateData,
+            templateDataMappings,
+            arbitratorExtraData,
+            registry,
+            feeToken
+        );
+        uint256 disputeID = 0;
+
+        vm.prank(staker1);
+        core.setStake(GENERAL_COURT, 20000);
+        vm.prank(disputer);
+        maliciousArbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action");
+        vm.warp(block.timestamp + minStakingTime);
+        sortitionModule.passPhase(); // Generating
+        vm.warp(block.timestamp + rngLookahead);
+        sortitionModule.passPhase(); // Drawing phase
+
+        core.draw(disputeID, DEFAULT_NB_OF_JURORS);
+        vm.warp(block.timestamp + timesPerPeriod[0]);
+        core.passPeriod(disputeID); // Vote
+
+        uint256[] memory voteIDs = new uint256[](3);
+        voteIDs[0] = 0;
+        voteIDs[1] = 1;
+        voteIDs[2] = 2;
+
+        vm.prank(staker1);
+        disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ");
+        core.passPeriod(disputeID); // Appeal
+
+        vm.warp(block.timestamp + timesPerPeriod[3]);
+        core.passPeriod(disputeID); // Execution
+
+        vm.expectRevert(MaliciousArbitrableMock.RuleReverted.selector);
+        core.executeRuling(disputeID); // Reverts
+
+        disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 2); // Should not revert even if executeRuling() reverted
+    }
+
     function test_executeRuling_appealSwitch() public {
         // Check that the ruling switches if only one side was funded
         uint256 disputeID = 0;
@@ -730,12 +773,13 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
         disputeKit.fundAppeal{value: 0.41 ether}(disputeID, 2); // Underpay a bit to not create an appeal and withdraw the funded sum fully
 
         vm.warp(block.timestamp + timesPerPeriod[3]);
-        core.passPeriod(disputeID); // Execution
 
         vm.expectRevert(DisputeKitClassicBase.DisputeNotResolved.selector);
         disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 1);
 
-        core.executeRuling(disputeID);
+        core.passPeriod(disputeID); // Execution
+        // executeRuling() should be irrelevant for withdrawals in case malicious arbitrable reverts rule()
+        //core.executeRuling(disputeID);
 
         vm.prank(owner);
         core.pause();
@@ -769,22 +813,15 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
         // 4. move sortition to staking phase
         uint256 disputeID = 0;
         uint256 amountToStake = 20000;
-        _stakePnk_createDispute_moveToDrawingPhase(disputeID, staker1, amountToStake);
+        _stakePnk_createDispute_moveToDrawingPhase(staker1, amountToStake);
 
         KlerosCore.Round memory round = core.getRoundInfo(disputeID, 0);
         uint256 pnkAtStakePerJuror = round.pnkAtStakePerJuror;
         _stakeBalanceForJuror(staker1, type(uint256).max);
         _drawJurors_advancePeriodToVoting(disputeID);
         _vote_execute(disputeID, staker1);
         sortitionModule.passPhase(); // set it to staking phase
-        _assertJurorBalance(
-            disputeID,
-            staker1,
-            amountToStake,
-            pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS,
-            amountToStake,
-            1
-        );
+        _assertJurorBalance(staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1);
 
         console.log("totalStaked before: %e", sortitionModule.totalStaked());
 
@@ -793,14 +830,7 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
 
         // post condition: inflated totalStaked
         console.log("totalStaked after: %e", sortitionModule.totalStaked());
-        _assertJurorBalance(
-            disputeID,
-            staker1,
-            amountToStake,
-            pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS,
-            amountToStake,
-            1
-        );
+        _assertJurorBalance(staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1);
 
         // new juror tries to stake but totalStaked already reached type(uint256).max
         // it reverts with "arithmetic underflow or overflow (0x11)"
@@ -909,19 +939,18 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
     ///////// Internal //////////
 
     function _assertJurorBalance(
-        uint256 disputeID,
-        address juror,
-        uint256 totalStakedPnk,
-        uint256 totalLocked,
-        uint256 stakedInCourt,
-        uint256 nbCourts
-    ) internal {
+        address _juror,
+        uint256 _totalStakedPnk,
+        uint256 _totalLocked,
+        uint256 _stakedInCourt,
+        uint256 _nbCourts
+    ) internal view {
         (uint256 totalStakedPnk, uint256 totalLocked, uint256 stakedInCourt, uint256 nbCourts) = sortitionModule
-            .getJurorBalance(juror, GENERAL_COURT);
-        assertEq(totalStakedPnk, totalStakedPnk, "Wrong totalStakedPnk"); // jurors total staked a.k.a juror.stakedPnk
-        assertEq(totalLocked, totalLocked, "Wrong totalLocked");
-        assertEq(stakedInCourt, stakedInCourt, "Wrong stakedInCourt"); // juror staked in court a.k.a _stakeOf
-        assertEq(nbCourts, nbCourts, "Wrong nbCourts");
+            .getJurorBalance(_juror, GENERAL_COURT);
+        assertEq(_totalStakedPnk, totalStakedPnk, "Wrong totalStakedPnk"); // jurors total staked a.k.a juror.stakedPnk
+        assertEq(_totalLocked, totalLocked, "Wrong totalLocked");
+        assertEq(_stakedInCourt, stakedInCourt, "Wrong stakedInCourt"); // juror staked in court a.k.a _stakeOf
+        assertEq(_nbCourts, nbCourts, "Wrong nbCourts");
     }
 
     function _stakeBalanceForJuror(address juror, uint256 amount) internal {
@@ -930,7 +959,7 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase {
         core.setStake(GENERAL_COURT, amount);
     }
 
-    function _stakePnk_createDispute_moveToDrawingPhase(uint256 disputeID, address juror, uint256 amount) internal {
+    function _stakePnk_createDispute_moveToDrawingPhase(address juror, uint256 amount) internal {
         vm.prank(juror);
         core.setStake(GENERAL_COURT, amount);
         vm.prank(disputer);

Original file line number	Diff line number	Diff line change
`@@ -149,7 +149,7 @@ contract ArbitrableExample is IArbitrableV2 {`
`149`	`149`	`}`
`150`	`150`
`151`	`151`	`/// @inheritdoc IArbitrableV2`
`152`		`- function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override {`
	`152`	`+ function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external virtual override {`
`153`	`153`	`uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];`
`154`	`154`	`DisputeStruct storage dispute = disputes[localDisputeID];`
`155`	`155`	`if (msg.sender != address(arbitrator)) revert ArbitratorOnly();`