Merge pull request #2154 from kleros/fix/contracts-review-round1

Contracts review feedback - round 1

Author: jaybuidl

contracts/src/arbitration/KlerosCore.sol

@@ -424,7 +424,8 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         sortitionModule = _sortitionModule;
     }
 
-    /// @notice Add a new supported dispute kit module to the court.
+    /// @notice Add a new supported dispute kit, without enabling it.
+    /// Use `enableDisputeKits()` to enable the dispute kit for a specific court.
     /// @param _disputeKitAddress The address of the dispute kit contract.
     function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner {
         uint256 disputeKitID = disputeKits.length;
@@ -461,7 +462,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         Court storage court = courts.push();
 
         for (uint256 i = 0; i < _supportedDisputeKits.length; i++) {
-            if (_supportedDisputeKits[i] == 0 || _supportedDisputeKits[i] >= disputeKits.length) {
+            if (_supportedDisputeKits[i] == NULL_DISPUTE_KIT || _supportedDisputeKits[i] >= disputeKits.length) {
                 revert WrongDisputeKitIndex();
             }
             _enableDisputeKit(uint96(courtID), _supportedDisputeKits[i], true);
@@ -483,7 +484,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         // Update the parent.
         courts[_parent].children.push(courtID);
         emit CourtCreated(
-            uint96(courtID),
+            courtID,
             _parent,
             _hiddenVotes,
             _minStake,
@@ -518,7 +519,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         }
         for (uint256 i = 0; i < court.children.length; i++) {
             if (courts[court.children[i]].minStake < _minStake) {
-                revert MinStakeLowerThanParentCourt();
+                revert MinStakeHigherThanChildCourt(court.children[i]);
             }
         }
         court.minStake = _minStake;
@@ -544,10 +545,10 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     /// @param _enable Whether add or remove the dispute kits from the court.
     function enableDisputeKits(uint96 _courtID, uint256[] memory _disputeKitIDs, bool _enable) external onlyByOwner {
         for (uint256 i = 0; i < _disputeKitIDs.length; i++) {
+            if (_disputeKitIDs[i] == NULL_DISPUTE_KIT || _disputeKitIDs[i] >= disputeKits.length) {
+                revert WrongDisputeKitIndex();
+            }
             if (_enable) {
-                if (_disputeKitIDs[i] == 0 || _disputeKitIDs[i] >= disputeKits.length) {
-                    revert WrongDisputeKitIndex();
-                }
                 _enableDisputeKit(_courtID, _disputeKitIDs[i], true);
             } else {
                 // Classic dispute kit must be supported by all courts.
@@ -940,7 +941,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         }
 
         if (pnkBalance == 0 || !disputeKit.isVoteActive(_params.disputeID, _params.round, _params.repartition)) {
-            // The juror is inactive or their balance is can't cover penalties anymore, unstake them from all courts.
+            // The juror is inactive or their balance can't cover penalties anymore, unstake them from all courts.
             sortitionModule.forcedUnstakeAllCourts(account);
         } else if (newCourtStake < courts[penalizedInCourtID].minStake) {
             // The juror's balance fell below the court minStake, unstake them from the court.
@@ -1365,7 +1366,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     function _extraDataToCourtIDMinJurorsDisputeKit(
         bytes memory _extraData
     ) internal view returns (uint96 courtID, uint256 minJurors, uint256 disputeKitID) {
-        // Note that if the extradata doesn't contain 32 bytes for the dispute kit ID it'll return the default 0 index.
+        // Note that if the _extraData doesn't contain 32 bytes, default values are used.
         if (_extraData.length >= 64) {
             assembly {
                 // solium-disable-line security/no-inline-assembly
@@ -1380,7 +1381,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
                 minJurors = DEFAULT_NB_OF_JURORS;
             }
             if (disputeKitID == NULL_DISPUTE_KIT || disputeKitID >= disputeKits.length) {
-                disputeKitID = DISPUTE_KIT_CLASSIC; // 0 index is not used.
+                disputeKitID = DISPUTE_KIT_CLASSIC;
             }
         } else {
             courtID = GENERAL_COURT;
@@ -1398,8 +1399,9 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     error DisputeKitOnly();
     error SortitionModuleOnly();
     error UnsuccessfulCall();
-    error InvalidDisputKitParent();
+    error InvalidDisputeKitParent();
     error MinStakeLowerThanParentCourt();
+    error MinStakeHigherThanChildCourt(uint256 _childCourtID);
     error UnsupportedDisputeKit();
     error InvalidForkingCourtAsParent();
     error WrongDisputeKitIndex();

contracts/src/arbitration/SortitionModule.sol

@@ -53,9 +53,9 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
     mapping(TreeKey key => SortitionTrees.Tree) sortitionSumTrees; // The mapping of sortition trees by keys.
     mapping(address account => Juror) public jurors; // The jurors.
     mapping(uint256 => DelayedStake) public delayedStakes; // Stores the stakes that were changed during Drawing phase, to update them when the phase is switched to Staking.
-    uint256 public maxStakePerJuror; // The maximum amount of PNK a juror can stake in a court.
-    uint256 public maxTotalStaked; // The maximum amount of PNK that can be staked in all courts.
-    uint256 public totalStaked; // The amount that is currently staked in all courts.
+    uint256 public maxStakePerJuror; // The maximum amount of PNK that a juror can stake across the courts.
+    uint256 public maxTotalStaked; // The maximum amount of PNK that all the jurors can stake across the courts.
+    uint256 public totalStaked; // The amount of PNK that is currently staked across the courts.
 
     // ************************************* //
     // *              Events               * //
@@ -105,8 +105,8 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
     /// @param _minStakingTime Minimal time to stake
     /// @param _maxDrawingTime Time after which the drawing phase can be switched
     /// @param _rng The random number generator.
-    /// @param _maxStakePerJuror The maximum amount of PNK a juror can stake in a court.
-    /// @param _maxTotalStaked The maximum amount of PNK that can be staked in all courts.
+    /// @param _maxStakePerJuror The maximum amount of PNK a juror can stake across the courts.
+    /// @param _maxTotalStaked The maximum amount of PNK that all the jurors can stake across the courts.
     function initialize(
         address _owner,
         KlerosCore _core,
@@ -343,14 +343,14 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
 
         if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply.
 
-        uint256 currentStake = _stakeOf(_account, _courtID);
+        uint256 currentStake = newCourtStake;
         uint256 newStake = 0;
         if (currentStake >= availablePenalty) {
             newStake = currentStake - availablePenalty;
         }
         _setStake(_account, _courtID, 0, availablePenalty, newStake);
         pnkBalance = juror.stakedPnk; // updated by _setStake()
-        newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake()
+        newCourtStake = newStake;
     }
 
     /// @inheritdoc ISortitionModule

contracts/src/arbitration/arbitrables/DisputeResolver.sol

@@ -30,12 +30,22 @@ contract DisputeResolver is IArbitrableV2 {
     DisputeStruct[] public disputes; // Local disputes.
     mapping(uint256 => uint256) public arbitratorDisputeIDToLocalID; // Maps arbitrator-side dispute IDs to local dispute IDs.
 
+    // ************************************* //
+    // *        Function Modifiers         * //
+    // ************************************* //
+
+    modifier onlyByOwner() {
+        if (owner != msg.sender) revert OwnerOnly();
+        _;
+    }
+
     // ************************************* //
     // *            Constructor            * //
     // ************************************* //
 
     /// @notice Constructor
     /// @param _arbitrator Target global arbitrator for any disputes.
+    /// @param _templateRegistry The dispute template registry.
     constructor(IArbitratorV2 _arbitrator, IDisputeTemplateRegistry _templateRegistry) {
         owner = msg.sender;
         arbitrator = _arbitrator;
@@ -48,18 +58,15 @@ contract DisputeResolver is IArbitrableV2 {
 
     /// @notice Changes the owner.
     /// @param _owner The address of the new owner.
-    function changeOwner(address _owner) external {
-        if (owner != msg.sender) revert OwnerOnly();
+    function changeOwner(address _owner) external onlyByOwner {
         owner = _owner;
     }
 
-    function changeArbitrator(IArbitratorV2 _arbitrator) external {
-        if (owner != msg.sender) revert OwnerOnly();
+    function changeArbitrator(IArbitratorV2 _arbitrator) external onlyByOwner {
         arbitrator = _arbitrator;
     }
 
-    function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external {
-        if (owner != msg.sender) revert OwnerOnly();
+    function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external onlyByOwner {
         templateRegistry = _templateRegistry;
     }
 

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol

@@ -370,7 +370,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
         if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();
 
         (uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID);
-        if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert AppealPeriodIsOver();
+        if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert NotAppealPeriod();
 
         uint256 multiplier;
         (uint256 ruling, , ) = this.currentRuling(_coreDisputeID);
@@ -381,7 +381,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
                 block.timestamp - appealPeriodStart >=
                 ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT
             ) {
-                revert AppealPeriodIsOverForLoser();
+                revert NotAppealPeriodForLoser();
             }
             multiplier = LOSER_STAKE_MULTIPLIER;
         }
@@ -759,8 +759,8 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
     error ChoiceOutOfBounds();
     error HashDoesNotMatchHiddenVoteCommitment();
     error VoteAlreadyCast();
-    error AppealPeriodIsOver();
-    error AppealPeriodIsOverForLoser();
+    error NotAppealPeriod();
+    error NotAppealPeriodForLoser();
     error AppealFeeIsAlreadyPaid();
     error DisputeNotResolved();
     error CoreIsPaused();

contracts/src/arbitration/interfaces/IArbitratorV2.sol

@@ -50,7 +50,7 @@ interface IArbitratorV2 {
     ) external payable returns (uint256 disputeID);
 
     /// @notice Create a dispute and pay for the fees in a supported ERC20 token.
-    /// @dev Must be called by the arbitrable contract and pay at least `arbitrationCost(_extraData)` in the supported ERC20 token.
+    /// @dev Must be called by the arbitrable contract and pay at least `arbitrationCost(_extraData, _feeToken)` in the supported ERC20 token.
     /// @param _numberOfChoices The number of choices the arbitrator can choose from in this dispute.
     /// @param _extraData Additional info about the dispute. We use it to pass the ID of the dispute's court (first 32 bytes), the minimum number of jurors required (next 32 bytes) and the ID of the specific dispute kit (last 32 bytes).
     /// @param _feeToken The ERC20 token used to pay fees.

contracts/src/arbitration/interfaces/IEvidence.sol

@@ -6,7 +6,7 @@ pragma solidity >=0.8.0 <0.9.0;
 interface IEvidence {
     /// @notice To be raised when evidence is submitted. Should point to the resource (evidences are not to be stored on chain due to gas considerations).
     /// @param _externalDisputeID Unique identifier for this dispute outside Kleros. It's the submitter responsibility to submit the right external dispute ID.
-    /// @param _party The address of the party submiting the evidence. Note that 0x0 refers to evidence not submitted by any party.
+    /// @param _party The address of the party submitting the evidence.
     /// @param _evidence Stringified evidence object, example: '{"name" : "Justification", "description" : "Description", "fileURI" : "/ipfs/QmWQV5ZFFhEJiW8Lm7ay2zLxC2XS4wx1b2W7FfdrLMyQQc"}'.
     event Evidence(uint256 indexed _externalDisputeID, address indexed _party, string _evidence);
 }

contracts/src/arbitration/interfaces/ISortitionModule.sol

@@ -78,7 +78,7 @@ interface ISortitionModule {
         uint256 _newStake
     ) external;
 
-    /// @notice Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution.
+    /// @notice Update the state of the stakes with a PNK penalty, called by KC during rewards execution.
     ///
     /// @dev `O(n + p * log_k(j))` where
     /// `n` is the number of courts the juror has staked in,

contracts/src/arbitration/university/KlerosCoreUniversity.sol

@@ -1173,7 +1173,7 @@ contract KlerosCoreUniversity is IArbitratorV2, UUPSProxiable, Initializable {
     error DisputeKitOnly();
     error SortitionModuleOnly();
     error UnsuccessfulCall();
-    error InvalidDisputKitParent();
+    error InvalidDisputeKitParent();
     error MinStakeLowerThanParentCourt();
     error UnsupportedDisputeKit();
     error InvalidForkingCourtAsParent();

contracts/test/foundry/KlerosCore_Appeals.t.sol

@@ -127,22 +127,22 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase {
         disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ");
 
         vm.prank(crowdfunder1);
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriod.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 1);
         core.passPeriod(disputeID);
 
         (uint256 start, uint256 end) = core.appealPeriod(0);
 
         vm.prank(crowdfunder1);
         vm.warp(block.timestamp + ((end - start) / 2 + 1));
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOverForLoser.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriodForLoser.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 1); // Losing choice
 
         disputeKit.fundAppeal(disputeID, 2); // Winning choice funding should not revert yet
 
         vm.prank(crowdfunder1);
         vm.warp(block.timestamp + (end - start) / 2); // Warp one more to cover the whole period
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriod.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2);
     }
 

contracts/test/foundry/KlerosCore_Governance.t.sol

@@ -310,7 +310,7 @@ contract KlerosCore_GovernanceTest is KlerosCore_TestBase {
             50, // jurors for jump
             [uint256(10), uint256(20), uint256(30), uint256(40)] // Times per period
         );
-        vm.expectRevert(KlerosCore.MinStakeLowerThanParentCourt.selector);
+        vm.expectRevert(abi.encodeWithSelector(KlerosCore.MinStakeHigherThanChildCourt.selector, newCourtID));
         vm.prank(owner);
         // Min stake of a parent became higher than of a child
         core.changeCourtParameters(