From 82f8b1cd345df493de4b2b368bb2f6bf392a26af Mon Sep 17 00:00:00 2001 From: unknownunknown1 Date: Wed, 13 Aug 2025 16:39:20 +1000 Subject: [PATCH 1/3] feat: replace requires with custom errors --- .../arbitration/DisputeTemplateRegistry.sol | 8 +- contracts/src/arbitration/KlerosGovernor.sol | 61 +++++++++----- contracts/src/arbitration/PolicyRegistry.sol | 8 +- .../src/arbitration/SortitionModuleBase.sol | 49 +++++++---- .../arbitrables/ArbitrableExample.sol | 23 ++++-- .../arbitrables/DisputeResolver.sol | 24 ++++-- .../devtools/DisputeResolverRuler.sol | 2 +- .../dispute-kits/DisputeKitClassicBase.sol | 82 ++++++++++++------- .../arbitration/evidence/EvidenceModule.sol | 8 +- .../university/SortitionModuleUniversity.sol | 14 +++- .../view/KlerosCoreSnapshotProxy.sol | 8 +- contracts/test/arbitration/index.ts | 6 +- contracts/test/arbitration/staking-neo.ts | 10 ++- contracts/test/foundry/KlerosCore.t.sol | 48 +++++------ 14 files changed, 234 insertions(+), 117 deletions(-) diff --git a/contracts/src/arbitration/DisputeTemplateRegistry.sol b/contracts/src/arbitration/DisputeTemplateRegistry.sol index 2ea0a71f6..e63d7c297 100644 --- a/contracts/src/arbitration/DisputeTemplateRegistry.sol +++ b/contracts/src/arbitration/DisputeTemplateRegistry.sol @@ -25,7 +25,7 @@ contract DisputeTemplateRegistry is IDisputeTemplateRegistry, UUPSProxiable, Ini // ************************************* // modifier onlyByGovernor() { - require(governor == msg.sender, "Governor only"); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -80,4 +80,10 @@ contract DisputeTemplateRegistry is IDisputeTemplateRegistry, UUPSProxiable, Ini templateId = templates++; emit DisputeTemplate(templateId, _templateTag, _templateData, _templateDataMappings); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); } diff --git a/contracts/src/arbitration/KlerosGovernor.sol b/contracts/src/arbitration/KlerosGovernor.sol index 7e9415a7b..a0a23061e 100644 --- a/contracts/src/arbitration/KlerosGovernor.sol +++ b/contracts/src/arbitration/KlerosGovernor.sol @@ -70,18 +70,18 @@ contract KlerosGovernor is IArbitrableV2 { modifier duringSubmissionPeriod() { uint256 offset = sessions[sessions.length - 1].durationOffset; - require(block.timestamp - lastApprovalTime <= submissionTimeout + offset, "Submission time has ended."); + if (block.timestamp - lastApprovalTime > submissionTimeout + offset) revert SubmissionTimeHasEnded(); _; } modifier duringApprovalPeriod() { uint256 offset = sessions[sessions.length - 1].durationOffset; - require(block.timestamp - lastApprovalTime > submissionTimeout + offset, "Approval time not started yet."); + if (block.timestamp - lastApprovalTime <= submissionTimeout + offset) revert ApprovalTimeNotStarted(); _; } modifier onlyByGovernor() { - require(address(this) == msg.sender, "Only the governor allowed."); + if (address(this) != msg.sender) revert GovernorOnly(); _; } @@ -208,14 +208,14 @@ contract KlerosGovernor is IArbitrableV2 { uint256[] memory _dataSize, string memory _description ) external payable duringSubmissionPeriod { - require(_target.length == _value.length, "Wrong input: target and value"); - require(_target.length == _dataSize.length, "Wrong input: target and datasize"); + if (_target.length != _value.length) revert WrongInputTargetAndValue(); + if (_target.length != _dataSize.length) revert WrongInputTargetAndDatasize(); Session storage session = sessions[sessions.length - 1]; Submission storage submission = submissions.push(); submission.submitter = payable(msg.sender); // Do the assignment first to avoid creating a new variable and bypass a 'stack too deep' error. submission.deposit = submissionBaseDeposit + arbitrator.arbitrationCost(arbitratorExtraData); - require(msg.value >= submission.deposit, "Not enough ETH to cover deposit"); + if (msg.value < submission.deposit) revert InsufficientDeposit(); bytes32 listHash; bytes32 currentTxHash; @@ -233,7 +233,7 @@ contract KlerosGovernor is IArbitrableV2 { currentTxHash = keccak256(abi.encodePacked(transaction.target, transaction.value, transaction.data)); listHash = keccak256(abi.encodePacked(currentTxHash, listHash)); } - require(!session.alreadySubmitted[listHash], "List already submitted"); + if (session.alreadySubmitted[listHash]) revert ListAlreadySubmitted(); session.alreadySubmitted[listHash] = true; submission.listHash = listHash; submission.submissionTime = block.timestamp; @@ -256,11 +256,11 @@ contract KlerosGovernor is IArbitrableV2 { function withdrawTransactionList(uint256 _submissionID, bytes32 _listHash) external { Session storage session = sessions[sessions.length - 1]; Submission storage submission = submissions[session.submittedLists[_submissionID]]; - require(block.timestamp - lastApprovalTime <= submissionTimeout / 2, "Should be in first half"); - // This require statement is an extra check to prevent _submissionID linking to the wrong list because of index swap during withdrawal. - require(submission.listHash == _listHash, "Wrong list hash"); - require(submission.submitter == msg.sender, "Only submitter can withdraw"); - require(block.timestamp - submission.submissionTime <= withdrawTimeout, "Withdrawing time has passed."); + if (block.timestamp - lastApprovalTime > submissionTimeout / 2) revert ShouldOnlyWithdrawInFirstHalf(); + // This is an extra check to prevent _submissionID linking to the wrong list because of index swap during withdrawal. + if (submission.listHash != _listHash) revert WrongListHash(); + if (submission.submitter != msg.sender) revert OnlySubmitterCanWithdraw(); + if (block.timestamp - submission.submissionTime > withdrawTimeout) revert WithdrawingTimeHasPassed(); session.submittedLists[_submissionID] = session.submittedLists[session.submittedLists.length - 1]; session.alreadySubmitted[_listHash] = false; session.submittedLists.pop(); @@ -273,7 +273,7 @@ contract KlerosGovernor is IArbitrableV2 { /// If nothing was submitted changes session. function executeSubmissions() external duringApprovalPeriod { Session storage session = sessions[sessions.length - 1]; - require(session.status == Status.NoDispute, "Already disputed"); + if (session.status != Status.NoDispute) revert AlreadyDisputed(); if (session.submittedLists.length == 0) { lastApprovalTime = block.timestamp; session.status = Status.Resolved; @@ -310,9 +310,9 @@ contract KlerosGovernor is IArbitrableV2 { /// Note If the final ruling is "0" nothing is approved and deposits will stay locked in the contract. function rule(uint256 _disputeID, uint256 _ruling) external override { Session storage session = sessions[sessions.length - 1]; - require(msg.sender == address(arbitrator), "Only arbitrator allowed"); - require(session.status == Status.DisputeCreated, "Wrong status"); - require(_ruling <= session.submittedLists.length, "Ruling is out of bounds."); + if (msg.sender != address(arbitrator)) revert OnlyArbitratorAllowed(); + if (session.status != Status.DisputeCreated) revert NotDisputed(); + if (_ruling > session.submittedLists.length) revert RulingOutOfBounds(); if (_ruling != 0) { Submission storage submission = submissions[session.submittedLists[_ruling - 1]]; @@ -338,8 +338,8 @@ contract KlerosGovernor is IArbitrableV2 { /// @param _count Number of transactions to execute. Executes until the end if set to "0" or number higher than number of transactions in the list. function executeTransactionList(uint256 _listID, uint256 _cursor, uint256 _count) external { Submission storage submission = submissions[_listID]; - require(submission.approved, "Should be approved"); - require(block.timestamp - submission.approvalTime <= executionTimeout, "Time to execute has passed"); + if (!submission.approved) revert SubmissionNotApproved(); + if (block.timestamp - submission.approvalTime > executionTimeout) revert TimeToExecuteHasPassed(); for (uint256 i = _cursor; i < submission.txs.length && (_count == 0 || i < _cursor + _count); i++) { Transaction storage transaction = submission.txs[i]; uint256 expendableFunds = getExpendableFunds(); @@ -347,7 +347,7 @@ contract KlerosGovernor is IArbitrableV2 { (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data); // An extra check to prevent re-entrancy through target call. if (callResult == true) { - require(!transaction.executed, "Already executed"); + if (transaction.executed) revert AlreadyExecuted(); transaction.executed = true; } } @@ -407,4 +407,27 @@ contract KlerosGovernor is IArbitrableV2 { function getCurrentSessionNumber() external view returns (uint256) { return sessions.length - 1; } + + // ************************************* // + // * Errors * // + // ************************************* // + + error SubmissionTimeHasEnded(); + error ApprovalTimeNotStarted(); + error GovernorOnly(); + error WrongInputTargetAndValue(); + error WrongInputTargetAndDatasize(); + error InsufficientDeposit(); + error ListAlreadySubmitted(); + error ShouldOnlyWithdrawInFirstHalf(); + error WrongListHash(); + error OnlySubmitterCanWithdraw(); + error WithdrawingTimeHasPassed(); + error AlreadyDisputed(); + error OnlyArbitratorAllowed(); + error NotDisputed(); + error RulingOutOfBounds(); + error SubmissionNotApproved(); + error TimeToExecuteHasPassed(); + error AlreadyExecuted(); } diff --git a/contracts/src/arbitration/PolicyRegistry.sol b/contracts/src/arbitration/PolicyRegistry.sol index eb32476d6..f4ed36322 100644 --- a/contracts/src/arbitration/PolicyRegistry.sol +++ b/contracts/src/arbitration/PolicyRegistry.sol @@ -32,7 +32,7 @@ contract PolicyRegistry is UUPSProxiable, Initializable { /// @dev Requires that the sender is the governor. modifier onlyByGovernor() { - require(governor == msg.sender, "No allowed: governor only"); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -85,4 +85,10 @@ contract PolicyRegistry is UUPSProxiable, Initializable { policies[_courtID] = _policy; emit PolicyUpdate(_courtID, _courtName, policies[_courtID]); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); } diff --git a/contracts/src/arbitration/SortitionModuleBase.sol b/contracts/src/arbitration/SortitionModuleBase.sol index 577d9fd22..e49fd2c6c 100644 --- a/contracts/src/arbitration/SortitionModuleBase.sol +++ b/contracts/src/arbitration/SortitionModuleBase.sol @@ -122,12 +122,12 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr // ************************************* // modifier onlyByGovernor() { - require(address(governor) == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } modifier onlyByCore() { - require(address(core) == msg.sender, "Access not allowed: KlerosCore only."); + if (address(core) != msg.sender) revert KlerosCoreOnly(); _; } @@ -171,23 +171,19 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr function passPhase() external { if (phase == Phase.staking) { - require( - block.timestamp - lastPhaseChange >= minStakingTime, - "The minimum staking time has not passed yet." - ); - require(disputesWithoutJurors > 0, "There are no disputes that need jurors."); + if (block.timestamp - lastPhaseChange < minStakingTime) revert MinStakingTimeNotPassed(); + if (disputesWithoutJurors == 0) revert NoDisputesThatNeedJurors(); rng.requestRandomness(block.number + rngLookahead); randomNumberRequestBlock = block.number; phase = Phase.generating; } else if (phase == Phase.generating) { randomNumber = rng.receiveRandomness(randomNumberRequestBlock + rngLookahead); - require(randomNumber != 0, "Random number is not ready yet"); + if (randomNumber == 0) revert RandomNumberNotReady(); phase = Phase.drawing; } else if (phase == Phase.drawing) { - require( - disputesWithoutJurors == 0 || block.timestamp - lastPhaseChange >= maxDrawingTime, - "There are still disputes without jurors and the maximum drawing time has not passed yet." - ); + if (disputesWithoutJurors > 0 && block.timestamp - lastPhaseChange < maxDrawingTime) { + revert DisputesWithoutJurorsAndMaxDrawingTimeNotPassed(); + } phase = Phase.staking; } @@ -201,8 +197,8 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr function createTree(bytes32 _key, bytes memory _extraData) external override onlyByCore { SortitionSumTree storage tree = sortitionSumTrees[_key]; uint256 K = _extraDataToTreeK(_extraData); - require(tree.K == 0, "Tree already exists."); - require(K > 1, "K must be greater than one."); + if (tree.K != 0) revert TreeAlreadyExists(); + if (K <= 1) revert KMustBeGreaterThanOne(); tree.K = K; tree.nodes.push(0); } @@ -210,8 +206,8 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr /// @dev Executes the next delayed stakes. /// @param _iterations The number of delayed stakes to execute. function executeDelayedStakes(uint256 _iterations) external { - require(phase == Phase.staking, "Should be in Staking phase."); - require(delayedStakeWriteIndex >= delayedStakeReadIndex, "No delayed stake to execute."); + if (phase != Phase.staking) revert NotStakingPhase(); + if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute(); uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 @@ -420,7 +416,7 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr // Can withdraw the leftover PNK if fully unstaked, has no tokens locked and has positive balance. // This withdrawal can't be triggered by calling setStake() in KlerosCore because current stake is technically 0, thus it is done via separate function. uint256 amount = getJurorLeftoverPNK(_account); - require(amount > 0, "Not eligible for withdrawal."); + if (amount == 0) revert NotEligibleForWithdrawal(); jurors[_account].stakedPnk = 0; core.transferBySortitionModule(_account, amount); emit LeftoverPNKWithdrawn(_account, amount); @@ -444,7 +440,7 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr uint256 _coreDisputeID, uint256 _nonce ) public view override returns (address drawnAddress) { - require(phase == Phase.drawing, "Wrong phase."); + if (phase != Phase.drawing) revert NotDrawingPhase(); SortitionSumTree storage tree = sortitionSumTrees[_key]; if (tree.nodes[0] == 0) { @@ -692,4 +688,21 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr stakePathID := mload(ptr) } } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error KlerosCoreOnly(); + error MinStakingTimeNotPassed(); + error NoDisputesThatNeedJurors(); + error RandomNumberNotReady(); + error DisputesWithoutJurorsAndMaxDrawingTimeNotPassed(); + error TreeAlreadyExists(); + error KMustBeGreaterThanOne(); + error NotStakingPhase(); + error NoDelayedStakeToExecute(); + error NotEligibleForWithdrawal(); + error NotDrawingPhase(); } diff --git a/contracts/src/arbitration/arbitrables/ArbitrableExample.sol b/contracts/src/arbitration/arbitrables/ArbitrableExample.sol index 810688788..b7596566e 100644 --- a/contracts/src/arbitration/arbitrables/ArbitrableExample.sol +++ b/contracts/src/arbitration/arbitrables/ArbitrableExample.sol @@ -37,7 +37,7 @@ contract ArbitrableExample is IArbitrableV2 { // ************************************* // modifier onlyByGovernor() { - require(msg.sender == governor, "Only the governor allowed."); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -126,8 +126,8 @@ contract ArbitrableExample is IArbitrableV2 { uint256 localDisputeID = disputes.length; disputes.push(DisputeStruct({isRuled: false, ruling: 0, numberOfRulingOptions: numberOfRulingOptions})); - require(weth.safeTransferFrom(msg.sender, address(this), _feeInWeth), "Transfer failed"); - require(weth.increaseAllowance(address(arbitrator), _feeInWeth), "Allowance increase failed"); + if (!weth.safeTransferFrom(msg.sender, address(this), _feeInWeth)) revert TransferFailed(); + if (!weth.increaseAllowance(address(arbitrator), _feeInWeth)) revert AllowanceIncreaseFailed(); disputeID = arbitrator.createDispute(numberOfRulingOptions, arbitratorExtraData, weth, _feeInWeth); externalIDtoLocalID[disputeID] = localDisputeID; @@ -142,13 +142,24 @@ contract ArbitrableExample is IArbitrableV2 { function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override { uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; DisputeStruct storage dispute = disputes[localDisputeID]; - require(msg.sender == address(arbitrator), "Only the arbitrator can execute this."); - require(_ruling <= dispute.numberOfRulingOptions, "Invalid ruling."); - require(dispute.isRuled == false, "This dispute has been ruled already."); + 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); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error TransferFailed(); + error AllowanceIncreaseFailed(); + error ArbitratorOnly(); + error RulingOutOfBounds(); + error DisputeAlreadyRuled(); } diff --git a/contracts/src/arbitration/arbitrables/DisputeResolver.sol b/contracts/src/arbitration/arbitrables/DisputeResolver.sol index 8fa1da02f..7248356cd 100644 --- a/contracts/src/arbitration/arbitrables/DisputeResolver.sol +++ b/contracts/src/arbitration/arbitrables/DisputeResolver.sol @@ -48,17 +48,17 @@ contract DisputeResolver is IArbitrableV2 { /// @dev Changes the governor. /// @param _governor The address of the new governor. function changeGovernor(address _governor) external { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); governor = _governor; } function changeArbitrator(IArbitratorV2 _arbitrator) external { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); arbitrator = _arbitrator; } function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); templateRegistry = _templateRegistry; } @@ -109,9 +109,9 @@ contract DisputeResolver is IArbitrableV2 { function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override { uint256 localDisputeID = arbitratorDisputeIDToLocalID[_arbitratorDisputeID]; DisputeStruct storage dispute = disputes[localDisputeID]; - require(msg.sender == address(arbitrator), "Only the arbitrator can execute this."); - require(_ruling <= dispute.numberOfRulingOptions, "Invalid ruling."); - require(!dispute.isRuled, "This dispute has been ruled already."); + if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); + if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds(); + if (dispute.isRuled) revert DisputeAlreadyRuled(); dispute.isRuled = true; dispute.ruling = _ruling; @@ -130,7 +130,7 @@ contract DisputeResolver is IArbitrableV2 { string memory _disputeTemplateUri, uint256 _numberOfRulingOptions ) internal virtual returns (uint256 arbitratorDisputeID) { - require(_numberOfRulingOptions > 1, "Should be at least 2 ruling options."); + if (_numberOfRulingOptions <= 1) revert ShouldBeAtLeastTwoRulingOptions(); arbitratorDisputeID = arbitrator.createDispute{value: msg.value}(_numberOfRulingOptions, _arbitratorExtraData); uint256 localDisputeID = disputes.length; @@ -146,4 +146,14 @@ contract DisputeResolver is IArbitrableV2 { uint256 templateId = templateRegistry.setDisputeTemplate("", _disputeTemplate, _disputeTemplateDataMappings); emit DisputeRequest(arbitrator, arbitratorDisputeID, localDisputeID, templateId, _disputeTemplateUri); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error ArbitratorOnly(); + error RulingOutOfBounds(); + error DisputeAlreadyRuled(); + error ShouldBeAtLeastTwoRulingOptions(); } diff --git a/contracts/src/arbitration/devtools/DisputeResolverRuler.sol b/contracts/src/arbitration/devtools/DisputeResolverRuler.sol index 85bede8bf..05c033c38 100644 --- a/contracts/src/arbitration/devtools/DisputeResolverRuler.sol +++ b/contracts/src/arbitration/devtools/DisputeResolverRuler.sol @@ -36,7 +36,7 @@ contract DisputeResolverRuler is DisputeResolver { string memory _disputeTemplateUri, uint256 _numberOfRulingOptions ) internal override returns (uint256 arbitratorDisputeID) { - require(_numberOfRulingOptions > 1, "Should be at least 2 ruling options."); + if (_numberOfRulingOptions <= 1) revert ShouldBeAtLeastTwoRulingOptions(); uint256 localDisputeID = disputes.length; DisputeStruct storage dispute = disputes.push(); diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol b/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol index 9b1968400..2d804891a 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol @@ -125,17 +125,17 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi // ************************************* // modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } modifier onlyByCore() { - require(address(core) == msg.sender, "Access not allowed: KlerosCore only."); + if (address(core) != msg.sender) revert KlerosCoreOnly(); _; } modifier notJumped(uint256 _coreDisputeID) { - require(!disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped, "Dispute jumped to a parent DK!"); + if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; } @@ -171,7 +171,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi bytes memory _data ) external onlyByGovernor { (bool success, ) = _destination.call{value: _amount}(_data); - require(success, "Unsuccessful call"); + if (!success) revert UnsuccessfulCall(); } /// @dev Changes the `governor` storage variable. @@ -269,14 +269,14 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi bytes32 _commit ) internal notJumped(_coreDisputeID) { (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID); - require(period == KlerosCoreBase.Period.commit, "The dispute should be in Commit period."); - require(_commit != bytes32(0), "Empty commit."); - require(coreDisputeIDToActive[_coreDisputeID], "Not active for core dispute ID"); + if (period != KlerosCoreBase.Period.commit) revert NotCommitPeriod(); + if (_commit == bytes32(0)) revert EmptyCommit(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; Round storage round = dispute.rounds[dispute.rounds.length - 1]; for (uint256 i = 0; i < _voteIDs.length; i++) { - require(round.votes[_voteIDs[i]].account == msg.sender, "The caller has to own the vote."); + if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); round.votes[_voteIDs[i]].commit = _commit; } round.totalCommitted += _voteIDs.length; @@ -310,12 +310,12 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi address _juror ) internal notJumped(_coreDisputeID) { (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID); - require(period == KlerosCoreBase.Period.vote, "The dispute should be in Vote period."); - require(_voteIDs.length > 0, "No voteID provided"); - require(coreDisputeIDToActive[_coreDisputeID], "Not active for core dispute ID"); + if (period != KlerosCoreBase.Period.vote) revert NotVotePeriod(); + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; - require(_choice <= dispute.numberOfChoices, "Choice out of bounds"); + if (_choice > dispute.numberOfChoices) revert ChoiceOutOfBounds(); Round storage round = dispute.rounds[dispute.rounds.length - 1]; { @@ -325,12 +325,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi // Save the votes. for (uint256 i = 0; i < _voteIDs.length; i++) { - require(round.votes[_voteIDs[i]].account == _juror, "The juror has to own the vote."); - require( - !hiddenVotes || round.votes[_voteIDs[i]].commit == voteHash, - "The vote hash must match the commitment in courts with hidden votes." - ); - require(!round.votes[_voteIDs[i]].voted, "Vote already cast."); + if (round.votes[_voteIDs[i]].account != _juror) revert JurorHasToOwnTheVote(); + if (hiddenVotes && round.votes[_voteIDs[i]].commit != voteHash) + revert HashDoesNotMatchHiddenVoteCommitment(); + if (round.votes[_voteIDs[i]].voted) revert VoteAlreadyCast(); round.votes[_voteIDs[i]].choice = _choice; round.votes[_voteIDs[i]].voted = true; } @@ -361,29 +359,30 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi /// @param _choice A choice that receives funding. function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable notJumped(_coreDisputeID) { Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; - require(_choice <= dispute.numberOfChoices, "There is no such ruling to fund."); - require(coreDisputeIDToActive[_coreDisputeID], "Not active for core dispute ID"); + if (_choice > dispute.numberOfChoices) revert ChoiceOutOfBounds(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); (uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID); - require(block.timestamp >= appealPeriodStart && block.timestamp < appealPeriodEnd, "Appeal period is over."); + if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert AppealPeriodIsOver(); uint256 multiplier; (uint256 ruling, , ) = this.currentRuling(_coreDisputeID); if (ruling == _choice) { multiplier = WINNER_STAKE_MULTIPLIER; } else { - require( - block.timestamp - appealPeriodStart < - ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT, - "Appeal period is over for loser" - ); + if ( + block.timestamp - appealPeriodStart >= + ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT + ) { + revert AppealPeriodIsOverForLoser(); + } multiplier = LOSER_STAKE_MULTIPLIER; } Round storage round = dispute.rounds[dispute.rounds.length - 1]; uint256 coreRoundID = core.getNumberOfRounds(_coreDisputeID) - 1; - require(!round.hasPaid[_choice], "Appeal fee is already paid."); + if (round.hasPaid[_choice]) revert AppealFeeIsAlreadyPaid(); uint256 appealCost = core.appealCost(_coreDisputeID); uint256 totalCost = appealCost + (appealCost * multiplier) / ONE_BASIS_POINT; @@ -440,9 +439,9 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi uint256 _choice ) external returns (uint256 amount) { (, , , bool isRuled, ) = core.disputes(_coreDisputeID); - require(isRuled, "Dispute should be resolved."); - require(!core.paused(), "Core is paused"); - require(coreDisputeIDToActive[_coreDisputeID], "Not active for core dispute ID"); + if (!isRuled) revert DisputeNotResolved(); + if (core.paused()) revert CoreIsPaused(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; Round storage round = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID]]; @@ -710,4 +709,27 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi result = true; } } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error KlerosCoreOnly(); + error DisputeJumpedToParentDK(); + error UnsuccessfulCall(); + error NotCommitPeriod(); + error EmptyCommit(); + error NotActiveForCoreDisputeID(); + error JurorHasToOwnTheVote(); + error NotVotePeriod(); + error EmptyVoteIDs(); + error ChoiceOutOfBounds(); + error HashDoesNotMatchHiddenVoteCommitment(); + error VoteAlreadyCast(); + error AppealPeriodIsOver(); + error AppealPeriodIsOverForLoser(); + error AppealFeeIsAlreadyPaid(); + error DisputeNotResolved(); + error CoreIsPaused(); } diff --git a/contracts/src/arbitration/evidence/EvidenceModule.sol b/contracts/src/arbitration/evidence/EvidenceModule.sol index fe55122b9..4967597ab 100644 --- a/contracts/src/arbitration/evidence/EvidenceModule.sol +++ b/contracts/src/arbitration/evidence/EvidenceModule.sol @@ -22,7 +22,7 @@ contract EvidenceModule is IEvidence, Initializable, UUPSProxiable { // ************************************* // modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -67,4 +67,10 @@ contract EvidenceModule is IEvidence, Initializable, UUPSProxiable { function submitEvidence(uint256 _externalDisputeID, string calldata _evidence) external { emit Evidence(_externalDisputeID, msg.sender, _evidence); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); } diff --git a/contracts/src/arbitration/university/SortitionModuleUniversity.sol b/contracts/src/arbitration/university/SortitionModuleUniversity.sol index 619a0b934..db61958fd 100644 --- a/contracts/src/arbitration/university/SortitionModuleUniversity.sol +++ b/contracts/src/arbitration/university/SortitionModuleUniversity.sol @@ -67,12 +67,12 @@ contract SortitionModuleUniversity is ISortitionModuleUniversity, UUPSProxiable, // ************************************* // modifier onlyByGovernor() { - require(address(governor) == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } modifier onlyByCore() { - require(address(core) == msg.sender, "Access not allowed: KlerosCore only."); + if (address(core) != msg.sender) revert KlerosCoreOnly(); _; } @@ -280,7 +280,7 @@ contract SortitionModuleUniversity is ISortitionModuleUniversity, UUPSProxiable, // Can withdraw the leftover PNK if fully unstaked, has no tokens locked and has positive balance. // This withdrawal can't be triggered by calling setStake() in KlerosCore because current stake is technically 0, thus it is done via separate function. uint256 amount = getJurorLeftoverPNK(_account); - require(amount > 0, "Not eligible for withdrawal."); + if (amount == 0) revert NotEligibleForWithdrawal(); jurors[_account].stakedPnk = 0; core.transferBySortitionModule(_account, amount); emit LeftoverPNKWithdrawn(_account, amount); @@ -364,4 +364,12 @@ contract SortitionModuleUniversity is ISortitionModuleUniversity, UUPSProxiable, } } } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error KlerosCoreOnly(); + error NotEligibleForWithdrawal(); } diff --git a/contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol b/contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol index e94eccbd9..74cdb84ce 100644 --- a/contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol +++ b/contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol @@ -26,7 +26,7 @@ contract KlerosCoreSnapshotProxy { // ************************************* // modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -69,4 +69,10 @@ contract KlerosCoreSnapshotProxy { function balanceOf(address _account) external view returns (uint256 totalStaked) { (totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0); } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); } diff --git a/contracts/test/arbitration/index.ts b/contracts/test/arbitration/index.ts index d8e7ef089..4af7dfd63 100644 --- a/contracts/test/arbitration/index.ts +++ b/contracts/test/arbitration/index.ts @@ -73,9 +73,9 @@ describe("DisputeKitClassic", async () => { }); it("Should create a dispute", async () => { - await expect(disputeKit.connect(deployer).createDispute(0, 0, ethers.toBeHex(3), "0x00")).to.be.revertedWith( - "Access not allowed: KlerosCore only." - ); + await expect( + disputeKit.connect(deployer).createDispute(0, 0, ethers.toBeHex(3), "0x00") + ).to.be.revertedWithCustomError(disputeKit, "KlerosCoreOnly"); const tx = await core .connect(deployer) diff --git a/contracts/test/arbitration/staking-neo.ts b/contracts/test/arbitration/staking-neo.ts index 85b0dc884..2f1523519 100644 --- a/contracts/test/arbitration/staking-neo.ts +++ b/contracts/test/arbitration/staking-neo.ts @@ -263,7 +263,10 @@ describe("Staking", async () => { ); expect(await sortition.totalStaked()).to.be.equal(PNK(0)); await drawAndReachStakingPhaseFromGenerating(); - await expect(sortition.executeDelayedStakes(10)).to.revertedWith("No delayed stake to execute."); + await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( + sortition, + "NoDelayedStakeToExecute" + ); expect(await sortition.totalStaked()).to.be.equal(PNK(0)); }); @@ -327,7 +330,10 @@ describe("Staking", async () => { ); expect(await sortition.totalStaked()).to.be.equal(PNK(2000)); await drawAndReachStakingPhaseFromGenerating(); - await expect(sortition.executeDelayedStakes(10)).to.revertedWith("No delayed stake to execute."); + await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( + sortition, + "NoDelayedStakeToExecute" + ); expect(await sortition.totalStaked()).to.be.equal(PNK(2000)); }); diff --git a/contracts/test/foundry/KlerosCore.t.sol b/contracts/test/foundry/KlerosCore.t.sol index 20f8555a7..72a6a565c 100644 --- a/contracts/test/foundry/KlerosCore.t.sol +++ b/contracts/test/foundry/KlerosCore.t.sol @@ -1137,7 +1137,7 @@ contract KlerosCoreTest is Test { vm.prank(staker2); core.setStake(GENERAL_COURT, 10000); - vm.expectRevert(bytes("No delayed stake to execute.")); + vm.expectRevert(SortitionModuleBase.NoDelayedStakeToExecute.selector); sortitionModule.executeDelayedStakes(5); // Set the stake and create a dispute to advance the phase @@ -1150,7 +1150,7 @@ contract KlerosCoreTest is Test { uint256 disputeID = 0; core.draw(disputeID, DEFAULT_NB_OF_JURORS); - vm.expectRevert(bytes("Should be in Staking phase.")); + vm.expectRevert(SortitionModuleBase.NotStakingPhase.selector); sortitionModule.executeDelayedStakes(5); // Create delayed stake @@ -1270,14 +1270,14 @@ contract KlerosCoreTest is Test { assertEq(snapshotProxy.balanceOf(staker1), 12346, "Wrong stPNK balance"); vm.prank(other); - vm.expectRevert(bytes("Access not allowed: Governor only.")); + vm.expectRevert(KlerosCoreSnapshotProxy.GovernorOnly.selector); snapshotProxy.changeCore(IKlerosCore(other)); vm.prank(governor); snapshotProxy.changeCore(IKlerosCore(other)); assertEq(address(snapshotProxy.core()), other, "Wrong core in snapshot proxy after change"); vm.prank(other); - vm.expectRevert(bytes("Access not allowed: Governor only.")); + vm.expectRevert(KlerosCoreSnapshotProxy.GovernorOnly.selector); snapshotProxy.changeGovernor(other); vm.prank(governor); snapshotProxy.changeGovernor(other); @@ -1565,7 +1565,7 @@ contract KlerosCoreTest is Test { voteIDs[0] = 0; bytes32 commit; vm.prank(staker1); - vm.expectRevert(bytes("The dispute should be in Commit period.")); + vm.expectRevert(DisputeKitClassicBase.NotCommitPeriod.selector); disputeKit.castCommit(disputeID, voteIDs, commit); vm.expectRevert(KlerosCoreBase.EvidenceNotPassedAndNotAppeal.selector); @@ -1582,13 +1582,13 @@ contract KlerosCoreTest is Test { assertEq(lastPeriodChange, block.timestamp, "Wrong lastPeriodChange"); vm.prank(staker1); - vm.expectRevert(bytes("Empty commit.")); + vm.expectRevert(DisputeKitClassicBase.EmptyCommit.selector); disputeKit.castCommit(disputeID, voteIDs, commit); commit = keccak256(abi.encodePacked(YES, salt)); vm.prank(other); - vm.expectRevert(bytes("The caller has to own the vote.")); + vm.expectRevert(DisputeKitClassicBase.JurorHasToOwnTheVote.selector); disputeKit.castCommit(disputeID, voteIDs, commit); vm.prank(staker1); @@ -1626,11 +1626,11 @@ contract KlerosCoreTest is Test { // Check the require with the wrong choice and then with the wrong salt vm.prank(staker1); - vm.expectRevert(bytes("The vote hash must match the commitment in courts with hidden votes.")); + vm.expectRevert(DisputeKitClassicBase.HashDoesNotMatchHiddenVoteCommitment.selector); disputeKit.castVote(disputeID, voteIDs, 2, salt, "XYZ"); vm.prank(staker1); - vm.expectRevert(bytes("The vote hash must match the commitment in courts with hidden votes.")); + vm.expectRevert(DisputeKitClassicBase.HashDoesNotMatchHiddenVoteCommitment.selector); disputeKit.castVote(disputeID, voteIDs, YES, salt - 1, "XYZ"); vm.prank(staker1); @@ -1698,7 +1698,7 @@ contract KlerosCoreTest is Test { uint256[] memory voteIDs = new uint256[](0); vm.prank(staker1); - vm.expectRevert(bytes("The dispute should be in Vote period.")); + vm.expectRevert(DisputeKitClassicBase.NotVotePeriod.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); // Leave salt empty as not needed vm.expectRevert(KlerosCoreBase.DisputeStillDrawing.selector); @@ -1716,17 +1716,17 @@ contract KlerosCoreTest is Test { assertEq(lastPeriodChange, block.timestamp, "Wrong lastPeriodChange"); vm.prank(staker1); - vm.expectRevert(bytes("No voteID provided")); + vm.expectRevert(DisputeKitClassicBase.EmptyVoteIDs.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); voteIDs = new uint256[](1); voteIDs[0] = 0; // Split vote IDs to see how the winner changes vm.prank(staker1); - vm.expectRevert(bytes("Choice out of bounds")); + vm.expectRevert(DisputeKitClassicBase.ChoiceOutOfBounds.selector); disputeKit.castVote(disputeID, voteIDs, 2 + 1, 0, "XYZ"); vm.prank(other); - vm.expectRevert(bytes("The juror has to own the vote.")); + vm.expectRevert(DisputeKitClassicBase.JurorHasToOwnTheVote.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); vm.prank(staker1); @@ -1735,7 +1735,7 @@ contract KlerosCoreTest is Test { disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); vm.prank(staker1); - vm.expectRevert(bytes("Vote already cast.")); + vm.expectRevert(DisputeKitClassicBase.VoteAlreadyCast.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); ( @@ -1970,7 +1970,7 @@ contract KlerosCoreTest is Test { core.appeal{value: 0.21 ether}(disputeID, 2, arbitratorExtraData); vm.prank(crowdfunder1); - vm.expectRevert(bytes("There is no such ruling to fund.")); + vm.expectRevert(DisputeKitClassicBase.ChoiceOutOfBounds.selector); disputeKit.fundAppeal(disputeID, 3); vm.prank(crowdfunder1); @@ -1995,7 +1995,7 @@ contract KlerosCoreTest is Test { assertEq((disputeKit.getFundedChoices(disputeID))[0], 1, "Incorrect funded choice"); vm.prank(crowdfunder1); - vm.expectRevert(bytes("Appeal fee is already paid.")); + vm.expectRevert(DisputeKitClassicBase.AppealFeeIsAlreadyPaid.selector); disputeKit.fundAppeal(disputeID, 1); } @@ -2024,7 +2024,7 @@ contract KlerosCoreTest is Test { disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); vm.prank(crowdfunder1); - vm.expectRevert(bytes("Appeal period is over.")); // Appeal period not started yet + vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector); disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 1); core.passPeriod(disputeID); @@ -2032,14 +2032,14 @@ contract KlerosCoreTest is Test { vm.prank(crowdfunder1); vm.warp(block.timestamp + ((end - start) / 2 + 1)); - vm.expectRevert(bytes("Appeal period is over for loser")); + vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOverForLoser.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(bytes("Appeal period is over.")); + vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector); disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2); } @@ -2210,7 +2210,7 @@ contract KlerosCoreTest is Test { // Check jump modifier vm.prank(address(core)); - vm.expectRevert(bytes("Dispute jumped to a parent DK!")); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); newDisputeKit.draw(disputeID, 1); // And check that draw in the new round works @@ -2594,7 +2594,7 @@ contract KlerosCoreTest is Test { assertEq(pinakion.balanceOf(address(core)), 1000, "Wrong token balance of the core"); assertEq(pinakion.balanceOf(staker1), 999999999999999000, "Wrong token balance of staker1"); - vm.expectRevert(bytes("Not eligible for withdrawal.")); + vm.expectRevert(SortitionModuleBase.NotEligibleForWithdrawal.selector); sortitionModule.withdrawLeftoverPNK(staker1); vm.expectEmit(true, true, true, true); @@ -2863,14 +2863,14 @@ contract KlerosCoreTest is Test { vm.warp(block.timestamp + timesPerPeriod[3]); core.passPeriod(disputeID); // Execution - vm.expectRevert(bytes("Dispute should be resolved.")); + vm.expectRevert(DisputeKitClassicBase.DisputeNotResolved.selector); disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 0, 1); core.executeRuling(disputeID); vm.prank(governor); core.pause(); - vm.expectRevert(bytes("Core is paused")); + vm.expectRevert(DisputeKitClassicBase.CoreIsPaused.selector); disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 0, 1); vm.prank(governor); core.unpause(); @@ -2969,7 +2969,7 @@ contract KlerosCoreTest is Test { // Deliberately cast votes using the old DK to see if the exception will be caught. vm.prank(staker1); - vm.expectRevert(bytes("Not active for core dispute ID")); + vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); // And check the new DK. From a0cb09d84f622eb31d2fbf39aa817db7e1eda055 Mon Sep 17 00:00:00 2001 From: jaybuidl Date: Wed, 13 Aug 2025 19:50:03 +0100 Subject: [PATCH 2/3] feat: replace requires with custom errors for the gateways --- contracts/src/gateway/ForeignGateway.sol | 45 +++++++++++++++--------- contracts/src/gateway/HomeGateway.sol | 33 +++++++++++------ 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/contracts/src/gateway/ForeignGateway.sol b/contracts/src/gateway/ForeignGateway.sol index 5938da773..1e615936f 100644 --- a/contracts/src/gateway/ForeignGateway.sol +++ b/contracts/src/gateway/ForeignGateway.sol @@ -49,17 +49,16 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { // ************************************* // modifier onlyFromVea(address _messageSender) { - require( - veaOutbox == msg.sender || - (block.timestamp < deprecatedVeaOutboxExpiration && deprecatedVeaOutbox == msg.sender), - "Access not allowed: Vea Outbox only." - ); - require(_messageSender == homeGateway, "Access not allowed: HomeGateway only."); + if ( + veaOutbox != msg.sender && + (block.timestamp >= deprecatedVeaOutboxExpiration || deprecatedVeaOutbox != msg.sender) + ) revert VeaOutboxOnly(); + if (_messageSender != homeGateway) revert HomeGatewayMessageSenderOnly(); _; } modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -105,7 +104,7 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { /// @dev Changes the governor. /// @param _governor The address of the new governor. function changeGovernor(address _governor) external { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); governor = _governor; } @@ -122,7 +121,7 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { /// @dev Changes the home gateway. /// @param _homeGateway The address of the new home gateway. function changeHomeGateway(address _homeGateway) external { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) revert GovernorOnly(); homeGateway = _homeGateway; } @@ -143,7 +142,7 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { uint256 _choices, bytes calldata _extraData ) external payable override returns (uint256 disputeID) { - require(msg.value >= arbitrationCost(_extraData), "Not paid enough for arbitration"); + if (msg.value < arbitrationCost(_extraData)) revert ArbitrationFeesNotEnough(); disputeID = localDisputeID++; uint256 chainID; @@ -206,8 +205,8 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { ) external override onlyFromVea(_messageSender) { DisputeData storage dispute = disputeHashtoDisputeData[_disputeHash]; - require(dispute.id != 0, "Dispute does not exist"); - require(!dispute.ruled, "Cannot rule twice"); + if (dispute.id == 0) revert DisputeDoesNotExist(); + if (dispute.ruled) revert CannotRuleTwice(); dispute.ruled = true; dispute.relayer = _relayer; @@ -219,8 +218,8 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { /// @inheritdoc IForeignGateway function withdrawFees(bytes32 _disputeHash) external override { DisputeData storage dispute = disputeHashtoDisputeData[_disputeHash]; - require(dispute.id != 0, "Dispute does not exist"); - require(dispute.ruled, "Not ruled yet"); + if (dispute.id == 0) revert DisputeDoesNotExist(); + if (!dispute.ruled) revert NotRuledYet(); uint256 amount = dispute.paid; dispute.paid = 0; @@ -247,9 +246,9 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { revert("Not supported"); } - // ************************ // - // * Internal * // - // ************************ // + // ************************************* // + // * Internal * // + // ************************************* // function extraDataToCourtIDMinJurors( bytes memory _extraData @@ -268,4 +267,16 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable { minJurors = DEFAULT_NB_OF_JURORS; } } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error HomeGatewayMessageSenderOnly(); + error VeaOutboxOnly(); + error ArbitrationFeesNotEnough(); + error DisputeDoesNotExist(); + error CannotRuleTwice(); + error NotRuledYet(); } diff --git a/contracts/src/gateway/HomeGateway.sol b/contracts/src/gateway/HomeGateway.sol index 40a767790..2ef8e606f 100644 --- a/contracts/src/gateway/HomeGateway.sol +++ b/contracts/src/gateway/HomeGateway.sol @@ -45,7 +45,7 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { /// @dev Requires that the sender is the governor. modifier onlyByGovernor() { - require(governor == msg.sender, "No allowed: governor only"); + if (governor != msg.sender) revert GovernorOnly(); _; } @@ -129,8 +129,8 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { /// @inheritdoc IHomeGateway function relayCreateDispute(RelayCreateDisputeParams memory _params) external payable override { - require(feeToken == NATIVE_CURRENCY, "Fees paid in ERC20 only"); - require(_params.foreignChainID == foreignChainID, "Foreign chain ID not supported"); + if (feeToken != NATIVE_CURRENCY) revert FeesPaidInNativeCurrencyOnly(); + if (_params.foreignChainID != foreignChainID) revert ForeignChainIDNotSupported(); bytes32 disputeHash = keccak256( abi.encodePacked( @@ -144,7 +144,7 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { ) ); RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; - require(relayedData.relayer == address(0), "Dispute already relayed"); + if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData); disputeIDtoHash[disputeID] = disputeHash; @@ -167,8 +167,8 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { /// @inheritdoc IHomeGateway function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external { - require(feeToken != NATIVE_CURRENCY, "Fees paid in native currency only"); - require(_params.foreignChainID == foreignChainID, "Foreign chain ID not supported"); + if (feeToken == NATIVE_CURRENCY) revert FeesPaidInERC20Only(); + if (_params.foreignChainID != foreignChainID) revert ForeignChainIDNotSupported(); bytes32 disputeHash = keccak256( abi.encodePacked( @@ -182,10 +182,10 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { ) ); RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash]; - require(relayedData.relayer == address(0), "Dispute already relayed"); + if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed(); - require(feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount), "Transfer failed"); - require(feeToken.increaseAllowance(address(arbitrator), _feeAmount), "Allowance increase failed"); + if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed(); + if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed(); uint256 disputeID = arbitrator.createDispute(_params.choices, _params.extraData, feeToken, _feeAmount); disputeIDtoHash[disputeID] = disputeHash; @@ -209,7 +209,7 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { /// @inheritdoc IArbitrableV2 function rule(uint256 _disputeID, uint256 _ruling) external override { - require(msg.sender == address(arbitrator), "Only Arbitrator"); + if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); bytes32 disputeHash = disputeIDtoHash[_disputeID]; RelayedData memory relayedData = disputeHashtoRelayedData[disputeHash]; @@ -234,4 +234,17 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable { function receiverGateway() external view override returns (address) { return foreignGateway; } + + // ************************************* // + // * Errors * // + // ************************************* // + + error GovernorOnly(); + error ArbitratorOnly(); + error FeesPaidInERC20Only(); + error FeesPaidInNativeCurrencyOnly(); + error ForeignChainIDNotSupported(); + error DisputeAlreadyRelayed(); + error TransferFailed(); + error AllowanceIncreaseFailed(); } From 5353ccf3922180e052bbd868360b77d314eaed41 Mon Sep 17 00:00:00 2001 From: jaybuidl Date: Wed, 13 Aug 2025 19:52:40 +0100 Subject: [PATCH 3/3] chore: changelog --- contracts/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/CHANGELOG.md b/contracts/CHANGELOG.md index 800208b3e..0a0250ade 100644 --- a/contracts/CHANGELOG.md +++ b/contracts/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Common Changelog](https://common-changelog.org/). ### Changed +- **Breaking:** Replace `require()` with `revert()` and custom errors outside KlerosCore for consistency and smaller bytecode ([#2084](https://github.com/kleros/kleros-v2/issues/2084)) - Set the Hardhat Solidity version to v0.8.30 and enable the IR pipeline ([#2069](https://github.com/kleros/kleros-v2/issues/2069)) - Set the Foundry Solidity version to v0.8.30 and enable the IR pipeline ([#2073](https://github.com/kleros/kleros-v2/issues/2073)) - Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083))