From 18186241b650641898a33e3393d49221f2e64fca Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Fri, 19 Jul 2024 03:16:18 -0300 Subject: [PATCH 1/7] fix: request id validation --- .../modules/dispute/BondEscalationModule.sol | 41 ++++++---- .../BondEscalationResolutionModule.sol | 11 ++- .../resolution/ERC20ResolutionModule.sol | 17 +++- .../PrivateERC20ResolutionModule.sol | 13 ++- .../modules/dispute/IBondEscalationModule.sol | 4 + .../IBondEscalationResolutionModule.sol | 5 ++ .../resolution/IERC20ResolutionModule.sol | 5 ++ .../IPrivateERC20ResolutionModule.sol | 5 ++ .../dispute/BondEscalationModule.t.sol | 81 +++++++++++++++---- .../BondEscalationResolutionModule.t.sol | 55 ++++++++++--- .../resolution/ERC20ResolutionModule.t.sol | 32 +++++++- .../PrivateERC20ResolutionModule.t.sol | 43 +++++++++- 12 files changed, 250 insertions(+), 62 deletions(-) diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index 1e453b90..1b587d2e 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -33,24 +33,27 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); + RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); if (block.number > ORACLE.createdAt(_dispute.responseId) + _params.disputeWindow) { revert BondEscalationModule_DisputeWindowOver(); } - BondEscalation storage _escalation = _escalations[_dispute.requestId]; + BondEscalation storage _escalation = _escalations[_requestId]; bytes32 _disputeId = _getId(_dispute); _params.accountingExtension.bond({ _bonder: _dispute.disputer, - _requestId: _dispute.requestId, + _requestId: _requestId, _token: _params.bondToken, _amount: _params.bondSize }); emit ResponseDisputed({ - _requestId: _dispute.requestId, + _requestId: _requestId, _responseId: _dispute.responseId, _disputeId: _disputeId, _dispute: _dispute, @@ -63,7 +66,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (block.timestamp > _params.bondEscalationDeadline) revert BondEscalationModule_BondEscalationOver(); _escalation.status = BondEscalationStatus.Active; _escalation.disputeId = _disputeId; - emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, BondEscalationStatus.Active); + emit BondEscalationStatusUpdated(_requestId, _disputeId, BondEscalationStatus.Active); } else if (_disputeId != _escalation.disputeId) { ORACLE.escalateDispute(_request, _response, _dispute); } @@ -76,8 +79,11 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata, IOracle.Dispute calldata _dispute ) external onlyOracle { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); + RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); - BondEscalation storage _escalation = _escalations[_dispute.requestId]; + BondEscalation storage _escalation = _escalations[_requestId]; IOracle.DisputeStatus _disputeStatus = ORACLE.disputeStatus(_disputeId); BondEscalationStatus _newStatus; @@ -103,7 +109,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_escalation.amountOfPledgesForDispute + _escalation.amountOfPledgesAgainstDispute > 0) { _params.accountingExtension.onSettleBondEscalation({ - _requestId: _dispute.requestId, + _requestId: _requestId, _disputeId: _disputeId, _token: _params.bondToken, _amountPerPledger: _params.bondSize, @@ -112,7 +118,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _params.accountingExtension.release({ - _requestId: _dispute.requestId, + _requestId: _requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -134,7 +140,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { + FixedPointMathLib.mulDivDown(_pledgesForDispute, _params.bondSize, _pledgesAgainstDispute); _params.accountingExtension.onSettleBondEscalation({ - _requestId: _dispute.requestId, + _requestId: _requestId, _disputeId: _escalation.disputeId, _token: _params.bondToken, _amountPerPledger: _amountToPay, @@ -143,7 +149,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _params.accountingExtension.pay({ - _requestId: _dispute.requestId, + _requestId: _requestId, _payer: _won ? _dispute.proposer : _dispute.disputer, _receiver: _won ? _dispute.disputer : _dispute.proposer, _token: _params.bondToken, @@ -152,7 +158,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_won) { _params.accountingExtension.release({ - _requestId: _dispute.requestId, + _requestId: _requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -170,7 +176,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { // Refund the disputer, the bond escalation status stays Escalated _newStatus = BondEscalationStatus.Escalated; _params.accountingExtension.release({ - _requestId: _dispute.requestId, + _requestId: _requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -182,7 +188,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { _newStatus = _won ? BondEscalationStatus.DisputerWon : BondEscalationStatus.DisputerLost; _params.accountingExtension.pay({ - _requestId: _dispute.requestId, + _requestId: _requestId, _payer: _won ? _dispute.proposer : _dispute.disputer, _receiver: _won ? _dispute.disputer : _dispute.proposer, _token: _params.bondToken, @@ -191,7 +197,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_won) { _params.accountingExtension.release({ - _requestId: _dispute.requestId, + _requestId: _requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -201,7 +207,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _escalation.status = _newStatus; - emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, _newStatus); + emit BondEscalationStatusUpdated(_requestId, _disputeId, _newStatus); emit DisputeStatusChanged({_disputeId: _disputeId, _dispute: _dispute, _status: _disputeStatus}); } @@ -252,6 +258,8 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Dispute calldata _dispute ) external { bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); + RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); BondEscalation storage _escalation = _escalations[_requestId]; @@ -294,7 +302,10 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Dispute calldata _dispute, bool _forDispute ) internal view returns (RequestParameters memory _params) { - BondEscalation memory _escalation = _escalations[_dispute.requestId]; + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); + + BondEscalation memory _escalation = _escalations[_requestId]; if (_disputeId != _escalation.disputeId) { revert BondEscalationModule_InvalidDispute(); diff --git a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol index 4ce18f4a..be05a454 100644 --- a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol +++ b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol @@ -83,7 +83,8 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _dispute.requestId; + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); Escalation storage _escalation = escalations[_disputeId]; @@ -124,8 +125,10 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu /// @inheritdoc IBondEscalationResolutionModule function claimPledge(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); - bytes32 _requestId = _dispute.requestId; Escalation storage _escalation = escalations[_disputeId]; if (_escalation.resolution == Resolution.Unresolved) revert BondEscalationResolutionModule_NotResolved(); @@ -204,8 +207,10 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu uint256 _pledgeAmount, bool _pledgingFor ) internal { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); - bytes32 _requestId = _dispute.requestId; Escalation storage _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert BondEscalationResolutionModule_NotEscalated(); diff --git a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol index 6917b9f7..83604a05 100644 --- a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol @@ -56,6 +56,9 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { IOracle.Dispute calldata _dispute, uint256 _numberOfVotes ) public { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); Escalation memory _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert ERC20ResolutionModule_DisputeNotEscalated(); @@ -72,7 +75,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { _voters[_disputeId].add(msg.sender); escalations[_disputeId].totalVotes += _numberOfVotes; - _params.accountingExtension.bond(msg.sender, _dispute.requestId, _params.votingToken, _numberOfVotes); + _params.accountingExtension.bond(msg.sender, _requestId, _params.votingToken, _numberOfVotes); emit VoteCast(msg.sender, _disputeId, _numberOfVotes); } @@ -83,6 +86,9 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); + // Check disputeId actually exists and that it isn't resolved already if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert ERC20ResolutionModule_AlreadyResolved(); @@ -102,15 +108,18 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // Update status if (_quorumReached == 1) { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won); - emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won); + emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won); } else { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost); - emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost); + emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost); } } /// @inheritdoc IERC20ResolutionModule function claimVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); Escalation memory _escalation = escalations[_disputeId]; @@ -121,7 +130,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // Transfer the tokens back to the voter uint256 _amount = votes[_disputeId][msg.sender]; - _params.accountingExtension.release(msg.sender, _dispute.requestId, _params.votingToken, _amount); + _params.accountingExtension.release(msg.sender, _requestId, _params.votingToken, _amount); emit VoteClaimed(msg.sender, _disputeId, _amount); } diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index df7e06cd..3b992b9a 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -52,6 +52,9 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { /// @inheritdoc IPrivateERC20ResolutionModule function commitVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bytes32 _commitment) public { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) { @@ -78,6 +81,9 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { uint256 _numberOfVotes, bytes32 _salt ) public { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); + bytes32 _disputeId = _getId(_dispute); Escalation memory _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert PrivateERC20ResolutionModule_DisputeNotEscalated(); @@ -113,6 +119,9 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { + bytes32 _requestId = _getId(_request); + if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); + if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) { revert PrivateERC20ResolutionModule_AlreadyResolved(); @@ -134,10 +143,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { if (_quorumReached == 1) { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won); - emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won); + emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won); } else { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost); - emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost); + emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost); } address _voter; diff --git a/solidity/interfaces/modules/dispute/IBondEscalationModule.sol b/solidity/interfaces/modules/dispute/IBondEscalationModule.sol index 175a984f..8d6caaa4 100644 --- a/solidity/interfaces/modules/dispute/IBondEscalationModule.sol +++ b/solidity/interfaces/modules/dispute/IBondEscalationModule.sol @@ -50,6 +50,10 @@ interface IBondEscalationModule is IDisputeModule { ERRORS //////////////////////////////////////////////////////////////*/ + /** + * @notice Thrown when a request does not match a request id. + */ + error BondEscalationModule_InvalidRequestId(); /** * @notice Thrown when trying to escalate a dispute going through the bond escalation module before its deadline. */ diff --git a/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol b/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol index f7582e5e..294cb4f4 100644 --- a/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol @@ -73,6 +73,11 @@ interface IBondEscalationResolutionModule is IResolutionModule { ERRORS //////////////////////////////////////////////////////////////*/ + /** + * @notice Thrown when a request does not match a request id. + */ + error BondEscalationResolutionModule_InvalidRequestId(); + /** * @notice Thrown when the user tries to resolve a dispute that has already been resolved. */ diff --git a/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol b/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol index e586f6a4..4042aee9 100644 --- a/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol @@ -53,6 +53,11 @@ interface IERC20ResolutionModule is IResolutionModule { */ error ERC20ResolutionModule_OnlyDisputeModule(); + /** + * @notice Thrown when a request does not match a request id + */ + error ERC20ResolutionModule_InvalidRequestId(); + /** * @notice Throws if the dispute doesn't exist or has not been escalated */ diff --git a/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol b/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol index 27c3389c..de5c16b2 100644 --- a/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol @@ -45,6 +45,11 @@ interface IPrivateERC20ResolutionModule is IResolutionModule { ERRORS //////////////////////////////////////////////////////////////*/ + /** + * @notice Thrown when a request does not match a request id + */ + error PrivateERC20ResolutionModule_InvalidRequestId(); + /** * @notice Thrown when the dispute has not been escalated */ diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index 02f7b83d..b70a1ada 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -212,9 +212,10 @@ contract BondEscalationModule_Unit_EscalateDispute is BaseTest { { // Set _bondEscalationDeadline to be the current timestamp to reach the second condition. _params.bondEscalationDeadline = block.timestamp; - mockRequest.disputeModuleData = abi.encode(_params); + mockRequest.disputeModuleData = abi.encode(_params); bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -255,8 +256,8 @@ contract BondEscalationModule_Unit_EscalateDispute is BaseTest { _params.bondEscalationDeadline = block.timestamp - 1; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -295,9 +296,10 @@ contract BondEscalationModule_Unit_EscalateDispute is BaseTest { _params.tyingBuffer = 1000; // Set bond escalation deadline to be the current timestamp. We will warp this. _params.bondEscalationDeadline = block.timestamp; - mockRequest.disputeModuleData = abi.encode(_params); + mockRequest.disputeModuleData = abi.encode(_params); bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -407,6 +409,16 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest { bondEscalationModule.disputeResponse(_request, mockResponse, mockDispute); } + /** + * @notice Tests that disputeResponse reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId() public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + vm.prank(address(oracle)); + bondEscalationModule.disputeResponse(mockRequest, mockResponse, mockDispute); + } + /** * @notice Tests that disputeResponse reverts if the challenge period for the response is over. */ @@ -455,7 +467,6 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest { mockResponse.requestId = _requestId; bytes32 _responseId = _getId(mockResponse); - mockDispute.responseId = _responseId; mockDispute.requestId = _requestId; @@ -602,6 +613,16 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest { bondEscalationModule.onDisputeStatusChange(_disputeId, _request, mockResponse, mockDispute); } + /** + * @notice Tests that onDisputeStatusChange reverts if request ids do not match + */ + function test_revertIfInvalidRequestId(bytes32 _disputeId) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + vm.prank(address(oracle)); + bondEscalationModule.onDisputeStatusChange(_disputeId, mockRequest, mockResponse, mockDispute); + } + /** * @notice Tests that onDisputeStatusChange pays the proposer if the disputer lost */ @@ -871,6 +892,7 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest { // Set to Lost so the proposer and againstDisputePledgers win IOracle.DisputeStatus _status = IOracle.DisputeStatus.Lost; + mockRequest.disputeModuleData = abi.encode(_params); bytes32 _requestId = _getId(mockRequest); @@ -930,21 +952,27 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest { } contract BondEscalationModule_Unit_PledgeForDispute is BaseTest { + /** + * @notice Tests that pledgeForDispute reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId() public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + bondEscalationModule.pledgeForDispute(mockRequest, mockDispute); + } + /** * @notice Tests that pledgeForDispute reverts if the dispute is not going through the bond escalation mechanism. */ - function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess( - bytes32 _disputeId, - bytes32 _requestId, - IOracle.Request calldata _request - ) public { + function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess(bytes32 _disputeId) public { vm.assume(_disputeId > 0); + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; // Check: does it revert if the dispute is not escalated yet? vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidDispute.selector); - bondEscalationModule.pledgeForDispute(_request, mockDispute); + bondEscalationModule.pledgeForDispute(mockRequest, mockDispute); } /** @@ -1112,21 +1140,27 @@ contract BondEscalationModule_Unit_PledgeForDispute is BaseTest { } contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest { + /** + * @notice Tests that pledgeAgainstDispute reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId() public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + bondEscalationModule.pledgeAgainstDispute(mockRequest, mockDispute); + } + /** * @notice Tests that pledgeAgainstDispute reverts if the dispute is not going through the bond escalation mechanism. */ - function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess( - bytes32 _disputeId, - bytes32 _requestId, - IOracle.Request calldata _request - ) public { + function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess(bytes32 _disputeId) public { vm.assume(_disputeId > 0); + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; // Check: does it revert if the dispute is not escalated yet? vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidDispute.selector); - bondEscalationModule.pledgeAgainstDispute(_request, mockDispute); + bondEscalationModule.pledgeAgainstDispute(mockRequest, mockDispute); } /** @@ -1298,6 +1332,15 @@ contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest { } contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { + /** + * @notice Tests that settleBondEscalation reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId() public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + bondEscalationModule.settleBondEscalation(mockRequest, mockResponse, mockDispute); + } + /** * @notice Tests that settleBondEscalation reverts if someone tries to settle the escalation before the tying buffer * has elapsed. @@ -1310,6 +1353,9 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; mockRequest.disputeModuleData = abi.encode(_params); + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + // Check: does it revert if the bond escalation is not over? vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector); bondEscalationModule.settleBondEscalation(mockRequest, mockResponse, mockDispute); @@ -1328,6 +1374,7 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { mockRequest.disputeModuleData = abi.encode(_params); bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; vm.warp(_params.bondEscalationDeadline + _params.tyingBuffer + 1); @@ -1349,8 +1396,8 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; _params.tyingBuffer = 1000; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); diff --git a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol index 4f669c1e..a4035058 100644 --- a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol @@ -249,14 +249,16 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { // block.timestamp < _startTime + _timeUntilDeadline _startTime = uint128(block.timestamp - timeUntilDeadline + 1); - - (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); } function test_reverts( uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + module.pledgeForDispute(mockRequest, mockDispute, _pledgeAmount); + // 1. BondEscalationResolutionModule_NotEscalated (_requestId,, _disputeId) = _setResolutionModuleData(_params); @@ -382,6 +384,8 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { uint256 _pledgesFor = 0; uint256 _pledgesAgainst = _pledgeAmount * 200 / 300; + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Set all data module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -431,6 +435,8 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { uint256 _pledgesFor = 100_000; uint256 _pledgesAgainst = (_pledgeAmount + _pledgesFor) * 301 / 200; + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Set the data module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -476,6 +482,8 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { uint256 _pledgesFor = 100_000; uint256 _pledgesAgainst = (_pledgeAmount + _pledgesFor); + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Resetting the pledges values module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -526,14 +534,16 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { // block.timestamp < _startTime + _timeUntilDeadline _startTime = uint128(block.timestamp - timeUntilDeadline + 1); - - (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); } function test_reverts( uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + module.pledgeAgainstDispute(mockRequest, mockDispute, _pledgeAmount); + // 1. BondEscalationResolutionModule_NotEscalated (_requestId,, _disputeId) = _setResolutionModuleData(_params); @@ -658,6 +668,8 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { uint256 _pledgesAgainst = 0; uint256 _pledgesFor = _pledgeAmount * 200 / 300; + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Set all data module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -707,6 +719,8 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { uint256 _pledgesAgainst = 100_000; uint256 _pledgesFor = (_pledgeAmount + _pledgesAgainst) * 301 / 200; + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Set the data module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -752,6 +766,8 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { uint256 _pledgesAgainst = 100_000; uint256 _pledgesFor = (_pledgeAmount + _pledgesAgainst); + (_requestId,, _disputeId) = _setResolutionModuleData(requestParameters); + // Resetting the pledges values module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -804,12 +820,20 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest { the disputer. */ - function test_reverts(IBondEscalationResolutionModule.RequestParameters memory _params) - public - assumeFuzzable(address(_params.accountingExtension)) - { + function test_reverts( + bytes32 _disputeId, + IBondEscalationResolutionModule.RequestParameters memory _params + ) public assumeFuzzable(address(_params.accountingExtension)) { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + // 1. BondEscalationResolutionModule_AlreadyResolved - (bytes32 _requestId, bytes32 _responseId, bytes32 _disputeId) = _setResolutionModuleData(_params); + bytes32 _requestId; + bytes32 _responseId; + (_requestId, _responseId, _disputeId) = _setResolutionModuleData(_params); // Set mock escalation with resolution == DisputerWon module.forTest_setEscalation(_disputeId, IBondEscalationResolutionModule.Resolution.DisputerWon, 0, 0, 0); @@ -991,13 +1015,18 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest { contract BondEscalationResolutionModule_Unit_ClaimPledge is BaseTest { function test_reverts( - bytes32 _disputeId, uint256 _pledgesFor, uint256 _pledgesAgainst, uint128 _startTime, address _randomPledger, - IOracle.Request calldata _request - ) public { + IBondEscalationResolutionModule.RequestParameters memory _params + ) public assumeFuzzable(address(_params.accountingExtension)) { + // Check: does it revert if request ids do not match? + vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + module.claimPledge(mockRequest, mockDispute); + + (,, bytes32 _disputeId) = _setResolutionModuleData(_params); + // Set a mock escalation with resolution == Unresolved module.forTest_setEscalation( _disputeId, IBondEscalationResolutionModule.Resolution.Unresolved, _startTime, _pledgesFor, _pledgesAgainst @@ -1008,7 +1037,7 @@ contract BondEscalationResolutionModule_Unit_ClaimPledge is BaseTest { // Check: does it revert if trying to claim a pledge of a not resolved escalation? vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_NotResolved.selector); - module.claimPledge(_request, mockDispute); + module.claimPledge(mockRequest, mockDispute); } function test_disputerWon( diff --git a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol index 45ad4563..94a2dfee 100644 --- a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol @@ -202,6 +202,15 @@ contract ERC20ResolutionModule_Unit_CastVote is BaseTest { assertEq(module.votes(_disputeId, _voter), _amountOfVotes); } + /** + * @notice Test that `castVote` reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId(uint256 _numberOfVotes) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); + module.castVote(mockRequest, mockDispute, _numberOfVotes); + } + /** * @notice Test that `castVote` reverts if called with `_disputeId` of a non-escalated dispute. */ @@ -256,7 +265,6 @@ contract ERC20ResolutionModule_Unit_CastVote is BaseTest { timeUntilDeadline: votingTimeWindow }) ); - bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -289,7 +297,6 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { timeUntilDeadline: votingTimeWindow }) ); - bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -344,6 +351,16 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); } + /** + * @notice Test that `resolveDispute` reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId(bytes32 _disputeId) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } + /** * @notice Test that `resolveDispute` reverts if called during voting phase. */ @@ -363,7 +380,6 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { ); mockDispute.requestId = _getId(mockRequest); bytes32 _disputeId = _getId(mockDispute); - module.forTest_setStartTime(_disputeId, 500_000); _mockAndExpect( @@ -381,6 +397,15 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { } contract ERC20ResolutionModule_Unit_ClaimVote is BaseTest { + /** + * @notice Reverts if request ids do not match + */ + function test_revertIfInvalidRequestId() public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); + module.claimVote(mockRequest, mockDispute); + } + /** * @notice Reverts if the vote is still ongoing */ @@ -393,7 +418,6 @@ contract ERC20ResolutionModule_Unit_ClaimVote is BaseTest { timeUntilDeadline: 1000 }) ); - mockDispute.requestId = _getId(mockRequest); module.forTest_setStartTime(_getId(mockDispute), block.timestamp); diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index 11ebc992..dbd351a4 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -224,10 +224,20 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { } /** - * @notice Test that `commitVote` reverts if there is no dispute with the given`_disputeId` + * @notice Test that `commitVote` reverts if request ids do not match. */ - function test_revertIfNonExistentDispute(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfInvalidRequestId(bytes32 _commitment) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); + module.commitVote(mockRequest, mockDispute, _commitment); + } + + /** + * @notice Test that `commitVote` reverts if there is no dispute with the given`_disputeId`. + */ + function test_revertIfNonExistentDispute(bytes32 _commitment) public { // Compute proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -242,8 +252,9 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { /** * @notice Test that `commitVote` reverts if called with `_disputeId` of an already resolved dispute. */ - function test_revertIfAlreadyResolved(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfAlreadyResolved(bytes32 _commitment) public { // Computer proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -262,8 +273,9 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { /** * @notice Test that `commitVote` reverts if called with `_disputeId` of a non-escalated dispute. */ - function test_revertIfNotEscalated(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfNotEscalated(bytes32 _commitment) public { // Compute proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -375,10 +387,23 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { assertEq(_voterData.numOfVotes, _amountOfVotes); } + /** + * @notice Test that `revealVote` reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId(uint256 _numberOfVotes, bytes32 _salt) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); + module.revealVote(mockRequest, mockDispute, _numberOfVotes, _salt); + } + /** * @notice Test that `revealVote` reverts if called with `_disputeId` of a non-escalated dispute. */ function test_revertIfNotEscalated(uint256 _numberOfVotes, bytes32 _salt) public { + // Compute proper id + bytes32 _requestId = _getId(mockRequest); + mockDispute.requestId = _requestId; + // Check: does it revert if the dispute is not escalated? vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_DisputeNotEscalated.selector); module.revealVote(mockRequest, mockDispute, _numberOfVotes, _salt); @@ -560,6 +585,16 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); } + /** + * @notice Test that `resolveDispute` reverts if request ids do not match. + */ + function test_revertIfInvalidRequestId(bytes32 _disputeId) public { + // Check: does it revert if request ids do not match? + vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); + vm.prank(address(oracle)); + module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); + } + /** * @notice Test that `resolveDispute` reverts if called during committing or revealing time window. */ From d542d410fc619ad3290c334bfffaab262bda478f Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Fri, 19 Jul 2024 14:57:18 -0300 Subject: [PATCH 2/7] test: fix merged tests --- .../resolution/PrivateERC20ResolutionModule.t.sol | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index f6700c66..bacedb1b 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -254,8 +254,9 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { /** * @notice Test that `commitVote` reverts if called with `_disputeId` of an already active dispute. */ - function test_revertIfActive(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfActive(bytes32 _commitment) public { // Computer proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -274,8 +275,9 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { /** * @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute with no resolution. */ - function test_revertIfNoResolution(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfNoResolution(bytes32 _commitment) public { // Computer proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); @@ -296,8 +298,9 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { /** * @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute that has already been won. */ - function test_revertIfWon(bytes32 _requestId, bytes32 _commitment) public { + function test_revertIfWon(bytes32 _commitment) public { // Computer proper IDs + bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; bytes32 _disputeId = _getId(mockDispute); From 9796e99a1e7a2936a5a33ce11bc99e28585f19ea Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Fri, 19 Jul 2024 15:46:21 -0300 Subject: [PATCH 3/7] perf: avoid redundant validations in onlyOracle functions --- .../modules/dispute/BondEscalationModule.sol | 34 ++++++++----------- .../BondEscalationResolutionModule.sol | 5 +-- .../resolution/ERC20ResolutionModule.sol | 7 ++-- .../PrivateERC20ResolutionModule.sol | 7 ++-- .../dispute/BondEscalationModule.t.sol | 20 ----------- .../BondEscalationResolutionModule.t.sol | 18 +++------- .../resolution/ERC20ResolutionModule.t.sol | 10 ------ .../PrivateERC20ResolutionModule.t.sol | 10 ------ 8 files changed, 24 insertions(+), 87 deletions(-) diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index 1b587d2e..60b575f1 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -33,27 +33,24 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); - RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); if (block.number > ORACLE.createdAt(_dispute.responseId) + _params.disputeWindow) { revert BondEscalationModule_DisputeWindowOver(); } - BondEscalation storage _escalation = _escalations[_requestId]; + BondEscalation storage _escalation = _escalations[_dispute.requestId]; bytes32 _disputeId = _getId(_dispute); _params.accountingExtension.bond({ _bonder: _dispute.disputer, - _requestId: _requestId, + _requestId: _dispute.requestId, _token: _params.bondToken, _amount: _params.bondSize }); emit ResponseDisputed({ - _requestId: _requestId, + _requestId: _dispute.requestId, _responseId: _dispute.responseId, _disputeId: _disputeId, _dispute: _dispute, @@ -66,7 +63,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (block.timestamp > _params.bondEscalationDeadline) revert BondEscalationModule_BondEscalationOver(); _escalation.status = BondEscalationStatus.Active; _escalation.disputeId = _disputeId; - emit BondEscalationStatusUpdated(_requestId, _disputeId, BondEscalationStatus.Active); + emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, BondEscalationStatus.Active); } else if (_disputeId != _escalation.disputeId) { ORACLE.escalateDispute(_request, _response, _dispute); } @@ -79,11 +76,8 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); - RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); - BondEscalation storage _escalation = _escalations[_requestId]; + BondEscalation storage _escalation = _escalations[_dispute.requestId]; IOracle.DisputeStatus _disputeStatus = ORACLE.disputeStatus(_disputeId); BondEscalationStatus _newStatus; @@ -109,7 +103,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_escalation.amountOfPledgesForDispute + _escalation.amountOfPledgesAgainstDispute > 0) { _params.accountingExtension.onSettleBondEscalation({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _token: _params.bondToken, _amountPerPledger: _params.bondSize, @@ -118,7 +112,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _params.accountingExtension.release({ - _requestId: _requestId, + _requestId: _dispute.requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -140,7 +134,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { + FixedPointMathLib.mulDivDown(_pledgesForDispute, _params.bondSize, _pledgesAgainstDispute); _params.accountingExtension.onSettleBondEscalation({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _escalation.disputeId, _token: _params.bondToken, _amountPerPledger: _amountToPay, @@ -149,7 +143,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _params.accountingExtension.pay({ - _requestId: _requestId, + _requestId: _dispute.requestId, _payer: _won ? _dispute.proposer : _dispute.disputer, _receiver: _won ? _dispute.disputer : _dispute.proposer, _token: _params.bondToken, @@ -158,7 +152,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_won) { _params.accountingExtension.release({ - _requestId: _requestId, + _requestId: _dispute.requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -176,7 +170,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { // Refund the disputer, the bond escalation status stays Escalated _newStatus = BondEscalationStatus.Escalated; _params.accountingExtension.release({ - _requestId: _requestId, + _requestId: _dispute.requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -188,7 +182,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { _newStatus = _won ? BondEscalationStatus.DisputerWon : BondEscalationStatus.DisputerLost; _params.accountingExtension.pay({ - _requestId: _requestId, + _requestId: _dispute.requestId, _payer: _won ? _dispute.proposer : _dispute.disputer, _receiver: _won ? _dispute.disputer : _dispute.proposer, _token: _params.bondToken, @@ -197,7 +191,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_won) { _params.accountingExtension.release({ - _requestId: _requestId, + _requestId: _dispute.requestId, _bonder: _dispute.disputer, _token: _params.bondToken, _amount: _params.bondSize @@ -207,7 +201,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } _escalation.status = _newStatus; - emit BondEscalationStatusUpdated(_requestId, _disputeId, _newStatus); + emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, _newStatus); emit DisputeStatusChanged({_disputeId: _disputeId, _dispute: _dispute, _status: _disputeStatus}); } diff --git a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol index be05a454..7eed3c0e 100644 --- a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol +++ b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol @@ -83,9 +83,6 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); - Escalation storage _escalation = escalations[_disputeId]; if (_escalation.resolution != Resolution.Unresolved) revert BondEscalationResolutionModule_AlreadyResolved(); @@ -120,7 +117,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu } ORACLE.updateDisputeStatus(_request, _response, _dispute, _disputeStatus); - emit DisputeResolved(_requestId, _disputeId, _disputeStatus); + emit DisputeResolved(_dispute.requestId, _disputeId, _disputeStatus); } /// @inheritdoc IBondEscalationResolutionModule diff --git a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol index 83604a05..31cde7b0 100644 --- a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol @@ -86,9 +86,6 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); - // Check disputeId actually exists and that it isn't resolved already if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert ERC20ResolutionModule_AlreadyResolved(); @@ -108,10 +105,10 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // Update status if (_quorumReached == 1) { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won); - emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won); + emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won); } else { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost); - emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost); + emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost); } } diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index c0f0a6b2..1af8f6d4 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -119,9 +119,6 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); - if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert PrivateERC20ResolutionModule_AlreadyResolved(); @@ -143,10 +140,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { if (_quorumReached == 1) { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won); - emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Won); + emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won); } else { ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Lost); - emit DisputeResolved(_requestId, _disputeId, IOracle.DisputeStatus.Lost); + emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost); } address _voter; diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index b70a1ada..1ded3798 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -409,16 +409,6 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest { bondEscalationModule.disputeResponse(_request, mockResponse, mockDispute); } - /** - * @notice Tests that disputeResponse reverts if request ids do not match. - */ - function test_revertIfInvalidRequestId() public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); - vm.prank(address(oracle)); - bondEscalationModule.disputeResponse(mockRequest, mockResponse, mockDispute); - } - /** * @notice Tests that disputeResponse reverts if the challenge period for the response is over. */ @@ -613,16 +603,6 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest { bondEscalationModule.onDisputeStatusChange(_disputeId, _request, mockResponse, mockDispute); } - /** - * @notice Tests that onDisputeStatusChange reverts if request ids do not match - */ - function test_revertIfInvalidRequestId(bytes32 _disputeId) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); - vm.prank(address(oracle)); - bondEscalationModule.onDisputeStatusChange(_disputeId, mockRequest, mockResponse, mockDispute); - } - /** * @notice Tests that onDisputeStatusChange pays the proposer if the disputer lost */ diff --git a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol index a4035058..6d43d599 100644 --- a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol @@ -820,20 +820,12 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest { the disputer. */ - function test_reverts( - bytes32 _disputeId, - IBondEscalationResolutionModule.RequestParameters memory _params - ) public assumeFuzzable(address(_params.accountingExtension)) { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); - - vm.prank(address(oracle)); - module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); - + function test_reverts(IBondEscalationResolutionModule.RequestParameters memory _params) + public + assumeFuzzable(address(_params.accountingExtension)) + { // 1. BondEscalationResolutionModule_AlreadyResolved - bytes32 _requestId; - bytes32 _responseId; - (_requestId, _responseId, _disputeId) = _setResolutionModuleData(_params); + (bytes32 _requestId, bytes32 _responseId, bytes32 _disputeId) = _setResolutionModuleData(_params); // Set mock escalation with resolution == DisputerWon module.forTest_setEscalation(_disputeId, IBondEscalationResolutionModule.Resolution.DisputerWon, 0, 0, 0); diff --git a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol index 94a2dfee..11d513c1 100644 --- a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol @@ -351,16 +351,6 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); } - /** - * @notice Test that `resolveDispute` reverts if request ids do not match. - */ - function test_revertIfInvalidRequestId(bytes32 _disputeId) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); - vm.prank(address(oracle)); - module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); - } - /** * @notice Test that `resolveDispute` reverts if called during voting phase. */ diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index bacedb1b..2159db95 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -652,16 +652,6 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); } - /** - * @notice Test that `resolveDispute` reverts if request ids do not match. - */ - function test_revertIfInvalidRequestId(bytes32 _disputeId) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); - vm.prank(address(oracle)); - module.resolveDispute(_disputeId, mockRequest, mockResponse, mockDispute); - } - /** * @notice Test that `resolveDispute` reverts if called during committing or revealing time window. */ From f636e00f2ebcc7b4d4b367b21436900b97ab15cc Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Sat, 20 Jul 2024 04:04:10 -0300 Subject: [PATCH 4/7] fix: response id validation --- .../modules/dispute/BondEscalationModule.sol | 13 ++--- .../dispute/BondEscalationModule.t.sol | 55 ++++++++++++++++--- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index 60b575f1..97592634 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -33,15 +33,14 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external onlyOracle { + bytes32 _disputeId = _getId(_dispute); RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); + BondEscalation storage _escalation = _escalations[_dispute.requestId]; if (block.number > ORACLE.createdAt(_dispute.responseId) + _params.disputeWindow) { revert BondEscalationModule_DisputeWindowOver(); } - BondEscalation storage _escalation = _escalations[_dispute.requestId]; - bytes32 _disputeId = _getId(_dispute); - _params.accountingExtension.bond({ _bonder: _dispute.disputer, _requestId: _dispute.requestId, @@ -251,11 +250,9 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); - + bytes32 _disputeId = _validateDispute(_request, _response, _dispute); RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); - BondEscalation storage _escalation = _escalations[_requestId]; + BondEscalation storage _escalation = _escalations[_dispute.requestId]; if (block.timestamp <= _params.bondEscalationDeadline + _params.tyingBuffer) { revert BondEscalationModule_BondEscalationNotOver(); @@ -275,7 +272,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { bool _disputersWon = _pledgesForDispute > _pledgesAgainstDispute; _escalation.status = _disputersWon ? BondEscalationStatus.DisputerWon : BondEscalationStatus.DisputerLost; - emit BondEscalationStatusUpdated(_requestId, _escalation.disputeId, _escalation.status); + emit BondEscalationStatusUpdated(_dispute.requestId, _disputeId, _escalation.status); ORACLE.updateDisputeStatus( _request, _response, _dispute, _disputersWon ? IOracle.DisputeStatus.Won : IOracle.DisputeStatus.Lost diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index 1ded3798..5e5d73b4 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -1313,11 +1313,28 @@ contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest { contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { /** - * @notice Tests that settleBondEscalation reverts if request ids do not match. + * @notice Tests that settleBondEscalation reverts if the response body is invalid. */ - function test_revertIfInvalidRequestId() public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + function test_revertIfInvalidResponseBody() public { + bytes32 _requestId = _getId(mockRequest); + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; + + // Check: does it revert if the response body is invalid? + vm.expectRevert(IModule.Module_InvalidResponseBody.selector); + bondEscalationModule.settleBondEscalation(mockRequest, mockResponse, mockDispute); + } + + /** + * @notice Tests that settleBondEscalation reverts if the dispute body is invalid. + */ + function test_revertIfInvalidDisputeBody() public { + bytes32 _requestId = _getId(mockRequest); + mockResponse.requestId = _requestId; + + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); bondEscalationModule.settleBondEscalation(mockRequest, mockResponse, mockDispute); } @@ -1332,9 +1349,13 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.tyingBuffer = bound(_params.tyingBuffer, 0, type(uint128).max); _params.bondEscalationDeadline = block.timestamp; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + + mockResponse.requestId = _requestId; + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; // Check: does it revert if the bond escalation is not over? vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector); @@ -1352,9 +1373,13 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; _params.tyingBuffer = 1000; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + + mockResponse.requestId = _requestId; + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; vm.warp(_params.bondEscalationDeadline + _params.tyingBuffer + 1); @@ -1376,9 +1401,13 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; _params.tyingBuffer = 1000; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + + mockResponse.requestId = _requestId; + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; bytes32 _disputeId = _getId(mockDispute); vm.warp(_params.bondEscalationDeadline + _params.tyingBuffer + 1); @@ -1407,9 +1436,13 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; _params.tyingBuffer = 1000; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + + mockResponse.requestId = _requestId; + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; bytes32 _disputeId = _getId(mockDispute); vm.warp(_params.bondEscalationDeadline + _params.tyingBuffer + 1); @@ -1451,9 +1484,13 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { _params.bondEscalationDeadline = block.timestamp; _params.tyingBuffer = 1000; mockRequest.disputeModuleData = abi.encode(_params); - bytes32 _requestId = _getId(mockRequest); + + mockResponse.requestId = _requestId; + bytes32 _responseId = _getId(mockResponse); + mockDispute.requestId = _requestId; + mockDispute.responseId = _responseId; bytes32 _disputeId = _getId(mockDispute); vm.warp(_params.bondEscalationDeadline + _params.tyingBuffer + 1); From 498b51a7c26511ab1d9f086a6155810329fac251 Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Tue, 23 Jul 2024 23:53:56 -0300 Subject: [PATCH 5/7] build: update prophet-core-contracts package --- package.json | 2 +- .../contracts/modules/dispute/BondEscalationModule.sol | 2 +- .../test/unit/modules/dispute/BondEscalationModule.t.sol | 5 ----- yarn.lock | 8 ++++---- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 016c690e..ec445145 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "package.json": "sort-package-json" }, "dependencies": { - "@defi-wonderland/prophet-core-contracts": "0.0.0-4d4a4487", + "@defi-wonderland/prophet-core-contracts": "0.0.0-ad40b65b", "@openzeppelin/contracts": "4.9.5", "solmate": "https://github.com/transmissions11/solmate.git#bfc9c25865a274a7827fea5abf6e4fb64fc64e6c" }, diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index 97592634..e17b806f 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -250,7 +250,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { IOracle.Response calldata _response, IOracle.Dispute calldata _dispute ) external { - bytes32 _disputeId = _validateDispute(_request, _response, _dispute); + (, bytes32 _disputeId) = _validateResponseAndDispute(_request, _response, _dispute); RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); BondEscalation storage _escalation = _escalations[_dispute.requestId]; diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index 5e5d73b4..fb312c73 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -1316,11 +1316,6 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { * @notice Tests that settleBondEscalation reverts if the response body is invalid. */ function test_revertIfInvalidResponseBody() public { - bytes32 _requestId = _getId(mockRequest); - bytes32 _responseId = _getId(mockResponse); - mockDispute.requestId = _requestId; - mockDispute.responseId = _responseId; - // Check: does it revert if the response body is invalid? vm.expectRevert(IModule.Module_InvalidResponseBody.selector); bondEscalationModule.settleBondEscalation(mockRequest, mockResponse, mockDispute); diff --git a/yarn.lock b/yarn.lock index 1f51733f..7509b4e1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -192,10 +192,10 @@ dependencies: "@jridgewell/trace-mapping" "0.3.9" -"@defi-wonderland/prophet-core-contracts@0.0.0-4d4a4487": - version "0.0.0-4d4a4487" - resolved "https://registry.yarnpkg.com/@defi-wonderland/prophet-core-contracts/-/prophet-core-contracts-0.0.0-4d4a4487.tgz#a794df6bc6d61287710778fbb73d2bde30094238" - integrity sha512-wE7Y1EDlmZQwqyxfuQ7z+DcKWyNxkXTLWBV8vt0PSY/Z8o6yUBhLe/NXYAEjrCJc4yPjQbqfADYRTmATN3w8YQ== +"@defi-wonderland/prophet-core-contracts@0.0.0-ad40b65b": + version "0.0.0-ad40b65b" + resolved "https://registry.yarnpkg.com/@defi-wonderland/prophet-core-contracts/-/prophet-core-contracts-0.0.0-ad40b65b.tgz#948ae61dc947577831c292f7ad2337123250aada" + integrity sha512-Aq7eu3Du+pikOgQS4oGAxt+QfTiP9PohD5qI3k1hJ6/iqpYURb/Ui9BmQ+zCC2Hn6c86J77GFxKpFVNmF1+YSQ== "@defi-wonderland/solidity-utils@0.0.0-3e9c8e8b": version "0.0.0-3e9c8e8b" From afcf539374d76e71573712d9410939d47c542742 Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Wed, 24 Jul 2024 00:30:18 -0300 Subject: [PATCH 6/7] refactor: validate dispute through Module --- .../modules/dispute/BondEscalationModule.sol | 12 ++++------ .../BondEscalationResolutionModule.sol | 24 +++++++------------ .../resolution/ERC20ResolutionModule.sol | 14 ++++------- .../PrivateERC20ResolutionModule.sol | 10 ++------ .../modules/dispute/IBondEscalationModule.sol | 4 ---- .../IBondEscalationResolutionModule.sol | 5 ---- .../resolution/IERC20ResolutionModule.sol | 10 -------- .../IPrivateERC20ResolutionModule.sol | 5 ---- .../dispute/BondEscalationModule.t.sol | 24 ++++++++----------- .../BondEscalationResolutionModule.t.sol | 12 +++++----- .../resolution/ERC20ResolutionModule.t.sol | 16 ++++++------- .../PrivateERC20ResolutionModule.t.sol | 16 ++++++------- 12 files changed, 51 insertions(+), 101 deletions(-) diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index e17b806f..3e4e0a42 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -211,7 +211,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { /// @inheritdoc IBondEscalationModule function pledgeForDispute(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { bytes32 _disputeId = _getId(_dispute); - RequestParameters memory _params = _pledgeChecks(_disputeId, _request, _dispute, true); + RequestParameters memory _params = _pledgeChecks(_request, _dispute, true); _escalations[_dispute.requestId].amountOfPledgesForDispute += 1; pledgesForDispute[_dispute.requestId][msg.sender] += 1; @@ -229,7 +229,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { /// @inheritdoc IBondEscalationModule function pledgeAgainstDispute(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { bytes32 _disputeId = _getId(_dispute); - RequestParameters memory _params = _pledgeChecks(_disputeId, _request, _dispute, false); + RequestParameters memory _params = _pledgeChecks(_request, _dispute, false); _escalations[_dispute.requestId].amountOfPledgesAgainstDispute += 1; pledgesAgainstDispute[_dispute.requestId][msg.sender] += 1; @@ -281,22 +281,18 @@ contract BondEscalationModule is Module, IBondEscalationModule { /** * @notice Checks the necessary conditions for pledging - * @param _disputeId The ID of the dispute to pledge for or against * @param _request The request data * @param _dispute The dispute data * @param _forDispute Whether the pledge is for or against the dispute * @return _params The decoded parameters for the request */ function _pledgeChecks( - bytes32 _disputeId, IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bool _forDispute ) internal view returns (RequestParameters memory _params) { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationModule_InvalidRequestId(); - - BondEscalation memory _escalation = _escalations[_requestId]; + bytes32 _disputeId = _validateDispute(_request, _dispute); + BondEscalation memory _escalation = _escalations[_dispute.requestId]; if (_disputeId != _escalation.disputeId) { revert BondEscalationModule_InvalidDispute(); diff --git a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol index 7eed3c0e..1246b792 100644 --- a/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol +++ b/solidity/contracts/modules/resolution/BondEscalationResolutionModule.sol @@ -122,10 +122,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu /// @inheritdoc IBondEscalationResolutionModule function claimPledge(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); Escalation storage _escalation = escalations[_disputeId]; if (_escalation.resolution == Resolution.Unresolved) revert BondEscalationResolutionModule_NotResolved(); @@ -143,7 +140,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu _reward = FixedPointMathLib.mulDivDown(_escalation.pledgesAgainst, _pledgerProportion, BASE); _amountToRelease = _reward + _pledgerBalanceBefore; _claimPledge({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _amountToRelease: _amountToRelease, _resolution: _escalation.resolution, @@ -156,7 +153,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu _reward = FixedPointMathLib.mulDivDown(_escalation.pledgesFor, _pledgerProportion, BASE); _amountToRelease = _reward + _pledgerBalanceBefore; _claimPledge({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _amountToRelease: _amountToRelease, _resolution: _escalation.resolution, @@ -169,7 +166,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu if (_pledgerBalanceFor > 0) { pledgesForDispute[_disputeId][msg.sender] -= _pledgerBalanceFor; _claimPledge({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _amountToRelease: _pledgerBalanceFor, _resolution: _escalation.resolution, @@ -180,7 +177,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu if (_pledgerBalanceAgainst > 0) { pledgesAgainstDispute[_disputeId][msg.sender] -= _pledgerBalanceAgainst; _claimPledge({ - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _amountToRelease: _pledgerBalanceAgainst, _resolution: _escalation.resolution, @@ -204,10 +201,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu uint256 _pledgeAmount, bool _pledgingFor ) internal { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert BondEscalationResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); Escalation storage _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert BondEscalationResolutionModule_NotEscalated(); @@ -226,7 +220,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu _params.accountingExtension.pledge({ _pledger: msg.sender, - _requestId: _requestId, + _requestId: _dispute.requestId, _disputeId: _disputeId, _token: _params.bondToken, _amount: _pledgeAmount @@ -239,7 +233,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu _escalation.pledgesFor += _pledgeAmount; pledgesForDispute[_disputeId][msg.sender] += _pledgeAmount; - emit PledgedForDispute(msg.sender, _requestId, _disputeId, _pledgeAmount); + emit PledgedForDispute(msg.sender, _dispute.requestId, _disputeId, _pledgeAmount); } else { if (_inequalityData.inequalityStatus == InequalityStatus.ForTurnToEqualize) { revert BondEscalationResolutionModule_ForTurnToEqualize(); @@ -247,7 +241,7 @@ contract BondEscalationResolutionModule is Module, IBondEscalationResolutionModu _escalation.pledgesAgainst += _pledgeAmount; pledgesAgainstDispute[_disputeId][msg.sender] += _pledgeAmount; - emit PledgedAgainstDispute(msg.sender, _requestId, _disputeId, _pledgeAmount); + emit PledgedAgainstDispute(msg.sender, _dispute.requestId, _disputeId, _pledgeAmount); } if (_escalation.pledgesFor + _escalation.pledgesAgainst >= _params.pledgeThreshold) { diff --git a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol index 31cde7b0..2f80211e 100644 --- a/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/ERC20ResolutionModule.sol @@ -56,10 +56,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { IOracle.Dispute calldata _dispute, uint256 _numberOfVotes ) public { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); Escalation memory _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert ERC20ResolutionModule_DisputeNotEscalated(); if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { @@ -75,7 +72,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { _voters[_disputeId].add(msg.sender); escalations[_disputeId].totalVotes += _numberOfVotes; - _params.accountingExtension.bond(msg.sender, _requestId, _params.votingToken, _numberOfVotes); + _params.accountingExtension.bond(msg.sender, _dispute.requestId, _params.votingToken, _numberOfVotes); emit VoteCast(msg.sender, _disputeId, _numberOfVotes); } @@ -114,10 +111,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { /// @inheritdoc IERC20ResolutionModule function claimVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute) external { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert ERC20ResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); Escalation memory _escalation = escalations[_disputeId]; // Check that voting deadline is over @@ -127,7 +121,7 @@ contract ERC20ResolutionModule is Module, IERC20ResolutionModule { // Transfer the tokens back to the voter uint256 _amount = votes[_disputeId][msg.sender]; - _params.accountingExtension.release(msg.sender, _requestId, _params.votingToken, _amount); + _params.accountingExtension.release(msg.sender, _dispute.requestId, _params.votingToken, _amount); emit VoteClaimed(msg.sender, _disputeId, _amount); } diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index 1af8f6d4..ff64a8fe 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -52,10 +52,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { /// @inheritdoc IPrivateERC20ResolutionModule function commitVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bytes32 _commitment) public { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute(); if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated) { revert PrivateERC20ResolutionModule_AlreadyResolved(); @@ -81,10 +78,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { uint256 _numberOfVotes, bytes32 _salt ) public { - bytes32 _requestId = _getId(_request); - if (_requestId != _dispute.requestId) revert PrivateERC20ResolutionModule_InvalidRequestId(); - - bytes32 _disputeId = _getId(_dispute); + bytes32 _disputeId = _validateDispute(_request, _dispute); Escalation memory _escalation = escalations[_disputeId]; if (_escalation.startTime == 0) revert PrivateERC20ResolutionModule_DisputeNotEscalated(); diff --git a/solidity/interfaces/modules/dispute/IBondEscalationModule.sol b/solidity/interfaces/modules/dispute/IBondEscalationModule.sol index 8d6caaa4..175a984f 100644 --- a/solidity/interfaces/modules/dispute/IBondEscalationModule.sol +++ b/solidity/interfaces/modules/dispute/IBondEscalationModule.sol @@ -50,10 +50,6 @@ interface IBondEscalationModule is IDisputeModule { ERRORS //////////////////////////////////////////////////////////////*/ - /** - * @notice Thrown when a request does not match a request id. - */ - error BondEscalationModule_InvalidRequestId(); /** * @notice Thrown when trying to escalate a dispute going through the bond escalation module before its deadline. */ diff --git a/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol b/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol index 294cb4f4..f7582e5e 100644 --- a/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IBondEscalationResolutionModule.sol @@ -73,11 +73,6 @@ interface IBondEscalationResolutionModule is IResolutionModule { ERRORS //////////////////////////////////////////////////////////////*/ - /** - * @notice Thrown when a request does not match a request id. - */ - error BondEscalationResolutionModule_InvalidRequestId(); - /** * @notice Thrown when the user tries to resolve a dispute that has already been resolved. */ diff --git a/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol b/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol index 4042aee9..86429c23 100644 --- a/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IERC20ResolutionModule.sol @@ -48,16 +48,6 @@ interface IERC20ResolutionModule is IResolutionModule { ERRORS //////////////////////////////////////////////////////////////*/ - /** - * @notice Throws if the caller is not the dispute module - */ - error ERC20ResolutionModule_OnlyDisputeModule(); - - /** - * @notice Thrown when a request does not match a request id - */ - error ERC20ResolutionModule_InvalidRequestId(); - /** * @notice Throws if the dispute doesn't exist or has not been escalated */ diff --git a/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol b/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol index de5c16b2..27c3389c 100644 --- a/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol +++ b/solidity/interfaces/modules/resolution/IPrivateERC20ResolutionModule.sol @@ -45,11 +45,6 @@ interface IPrivateERC20ResolutionModule is IResolutionModule { ERRORS //////////////////////////////////////////////////////////////*/ - /** - * @notice Thrown when a request does not match a request id - */ - error PrivateERC20ResolutionModule_InvalidRequestId(); - /** * @notice Thrown when the dispute has not been escalated */ diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index fb312c73..630b06e1 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -933,20 +933,18 @@ contract BondEscalationModule_Unit_OnDisputeStatusChange is BaseTest { contract BondEscalationModule_Unit_PledgeForDispute is BaseTest { /** - * @notice Tests that pledgeForDispute reverts if request ids do not match. + * @notice Tests that pledgeForDispute reverts if the dispute body is invalid. */ - function test_revertIfInvalidRequestId() public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody() public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); bondEscalationModule.pledgeForDispute(mockRequest, mockDispute); } /** * @notice Tests that pledgeForDispute reverts if the dispute is not going through the bond escalation mechanism. */ - function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess(bytes32 _disputeId) public { - vm.assume(_disputeId > 0); - + function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess() public { bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; @@ -1121,20 +1119,18 @@ contract BondEscalationModule_Unit_PledgeForDispute is BaseTest { contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest { /** - * @notice Tests that pledgeAgainstDispute reverts if request ids do not match. + * @notice Tests that pledgeAgainstDispute reverts if the dispute body is invalid. */ - function test_revertIfInvalidRequestId() public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationModule.BondEscalationModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody() public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); bondEscalationModule.pledgeAgainstDispute(mockRequest, mockDispute); } /** * @notice Tests that pledgeAgainstDispute reverts if the dispute is not going through the bond escalation mechanism. */ - function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess(bytes32 _disputeId) public { - vm.assume(_disputeId > 0); - + function test_revertIfTheDisputeIsNotGoingThroughTheBondEscalationProcess() public { bytes32 _requestId = _getId(mockRequest); mockDispute.requestId = _requestId; diff --git a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol index 6d43d599..562e6248 100644 --- a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol @@ -255,8 +255,8 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.pledgeForDispute(mockRequest, mockDispute, _pledgeAmount); // 1. BondEscalationResolutionModule_NotEscalated @@ -540,8 +540,8 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.pledgeAgainstDispute(mockRequest, mockDispute, _pledgeAmount); // 1. BondEscalationResolutionModule_NotEscalated @@ -1013,8 +1013,8 @@ contract BondEscalationResolutionModule_Unit_ClaimPledge is BaseTest { address _randomPledger, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { - // Check: does it revert if request ids do not match? - vm.expectRevert(IBondEscalationResolutionModule.BondEscalationResolutionModule_InvalidRequestId.selector); + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.claimPledge(mockRequest, mockDispute); (,, bytes32 _disputeId) = _setResolutionModuleData(_params); diff --git a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol index 11d513c1..ebbe6419 100644 --- a/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/ERC20ResolutionModule.t.sol @@ -203,11 +203,11 @@ contract ERC20ResolutionModule_Unit_CastVote is BaseTest { } /** - * @notice Test that `castVote` reverts if request ids do not match. + * @notice Test that `castVote` reverts if the dispute body is invalid. */ - function test_revertIfInvalidRequestId(uint256 _numberOfVotes) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody(uint256 _numberOfVotes) public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.castVote(mockRequest, mockDispute, _numberOfVotes); } @@ -388,11 +388,11 @@ contract ERC20ResolutionModule_Unit_ResolveDispute is BaseTest { contract ERC20ResolutionModule_Unit_ClaimVote is BaseTest { /** - * @notice Reverts if request ids do not match + * @notice Reverts if the dispute body is invalid */ - function test_revertIfInvalidRequestId() public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IERC20ResolutionModule.ERC20ResolutionModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody() public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.claimVote(mockRequest, mockDispute); } diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index 2159db95..bf628d53 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -226,11 +226,11 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { } /** - * @notice Test that `commitVote` reverts if request ids do not match. + * @notice Test that `commitVote` reverts if the dispute body is invalid. */ - function test_revertIfInvalidRequestId(bytes32 _commitment) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody(bytes32 _commitment) public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.commitVote(mockRequest, mockDispute, _commitment); } @@ -455,11 +455,11 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { } /** - * @notice Test that `revealVote` reverts if request ids do not match. + * @notice Test that `revealVote` reverts if the dispute body is invalid. */ - function test_revertIfInvalidRequestId(uint256 _numberOfVotes, bytes32 _salt) public { - // Check: does it revert if request ids do not match? - vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_InvalidRequestId.selector); + function test_revertIfInvalidDisputeBody(uint256 _numberOfVotes, bytes32 _salt) public { + // Check: does it revert if the dispute body is invalid? + vm.expectRevert(IModule.Module_InvalidDisputeBody.selector); module.revealVote(mockRequest, mockDispute, _numberOfVotes, _salt); } From 28628c3c8c1052e4e7eb46502ab815eef443c705 Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Thu, 25 Jul 2024 22:32:32 -0300 Subject: [PATCH 7/7] test: rename reversion test cases --- .../resolution/BondEscalationResolutionModule.t.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol index 562e6248..7eb0e1ac 100644 --- a/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/BondEscalationResolutionModule.t.sol @@ -251,7 +251,7 @@ contract BondEscalationResolutionModule_Unit_PledgeForDispute is BaseTest { _startTime = uint128(block.timestamp - timeUntilDeadline + 1); } - function test_reverts( + function test_pledgeForDisputeReverts( uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { @@ -536,7 +536,7 @@ contract BondEscalationResolutionModule_Unit_PledgeAgainstDispute is BaseTest { _startTime = uint128(block.timestamp - timeUntilDeadline + 1); } - function test_reverts( + function test_pledgeAgainstDisputeReverts( uint256 _pledgeAmount, IBondEscalationResolutionModule.RequestParameters memory _params ) public assumeFuzzable(address(_params.accountingExtension)) { @@ -820,7 +820,7 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest { the disputer. */ - function test_reverts(IBondEscalationResolutionModule.RequestParameters memory _params) + function test_resolveDisputeReverts(IBondEscalationResolutionModule.RequestParameters memory _params) public assumeFuzzable(address(_params.accountingExtension)) { @@ -1006,7 +1006,7 @@ contract BondEscalationResolutionModule_Unit_ResolveDispute is BaseTest { } contract BondEscalationResolutionModule_Unit_ClaimPledge is BaseTest { - function test_reverts( + function test_claimPledgeReverts( uint256 _pledgesFor, uint256 _pledgesAgainst, uint128 _startTime,