fix(KC): malicious arbitrable revert

Author: unknownunknown1

contracts/src/arbitration/KlerosCore.sol

@@ -139,6 +139,11 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     /// @param _voteID ID of the vote given to the drawn juror.
     event Draw(address indexed _address, uint256 indexed _disputeID, uint256 _roundID, uint256 _voteID);
 
+    /// @notice Emitted when the dispute is successfully appealed.
+    /// @param _disputeID ID of the related dispute.
+    /// @param _arbitrable The arbitrable contract.
+    event ArbitrableReverted(uint256 indexed _disputeID, IArbitrableV2 indexed _arbitrable);
+
     /// @notice Emitted when a new court is created.
     /// @param _courtID ID of the new court.
     /// @param _parent ID of the parent court.
@@ -1068,7 +1073,9 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         (uint256 winningChoice, , ) = currentRuling(_disputeID);
         dispute.ruled = true;
         emit Ruling(dispute.arbitrated, _disputeID, winningChoice);
-        dispute.arbitrated.rule(_disputeID, winningChoice);
+        try dispute.arbitrated.rule(_disputeID, winningChoice) {} catch {
+            emit ArbitrableReverted(_disputeID, dispute.arbitrated);
+        }
     }
 
     // ************************************* //

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 {
         uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
         DisputeStruct storage dispute = disputes[localDisputeID];
         if (msg.sender != address(arbitrator)) revert ArbitratorOnly();

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();
 

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();
+}

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
@@ -644,6 +645,60 @@ 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
+
+        core.executeRuling(disputeID);
+
+        (, , , bool ruled, ) = core.disputes(disputeID);
+        assertEq(ruled, true, "Should be ruled");
+
+        (bool isRuled, , ) = maliciousArbitrable.disputes(disputeID);
+        assertEq(isRuled, false, "Should be false");
+
+        vm.expectRevert(KlerosCore.RulingAlreadyExecuted.selector);
+        core.executeRuling(disputeID);
+
+        maliciousArbitrable.changeBehaviour(false);
+
+        // If the first revert was accidental arbitrable will be locked out.
+        vm.expectRevert(KlerosCore.RulingAlreadyExecuted.selector);
+        core.executeRuling(disputeID);
+    }
+
     function test_executeRuling_appealSwitch() public {
         // Check that the ruling switches if only one side was funded
         uint256 disputeID = 0;
@@ -719,12 +774,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();