From 679c9c324ec65f279df6d8a55abf6b28a08c63de Mon Sep 17 00:00:00 2001 From: shaito Date: Wed, 23 Oct 2024 13:11:52 +0200 Subject: [PATCH] feat: standardized timestamp comparison --- .../modules/dispute/BondEscalationModule.sol | 10 +-- .../PrivateERC20ResolutionModule.sol | 6 +- .../test/integration/EscalateDispute.t.sol | 2 +- .../dispute/BondEscalationModule.t.sol | 72 ++++++++++++++++++- .../PrivateERC20ResolutionModule.t.sol | 38 +++++----- 5 files changed, 99 insertions(+), 29 deletions(-) diff --git a/solidity/contracts/modules/dispute/BondEscalationModule.sol b/solidity/contracts/modules/dispute/BondEscalationModule.sol index 26b62cd2..e3e9f9c0 100644 --- a/solidity/contracts/modules/dispute/BondEscalationModule.sol +++ b/solidity/contracts/modules/dispute/BondEscalationModule.sol @@ -36,7 +36,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); BondEscalation storage _escalation = _escalations[_dispute.requestId]; - if (block.timestamp > ORACLE.responseCreatedAt(_dispute.responseId) + _params.disputeWindow) { + if (block.timestamp >= ORACLE.responseCreatedAt(_dispute.responseId) + _params.disputeWindow) { revert BondEscalationModule_DisputeWindowOver(); } @@ -82,7 +82,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { if (_disputeStatus == IOracle.DisputeStatus.Escalated) { // The dispute has been escalated to the Resolution module // Make sure the bond escalation deadline has passed and update the status - if (block.timestamp <= ORACLE.disputeCreatedAt(_disputeId) + _params.bondEscalationDeadline) { + if (block.timestamp < ORACLE.disputeCreatedAt(_disputeId) + _params.bondEscalationDeadline) { revert BondEscalationModule_BondEscalationNotOver(); } @@ -254,7 +254,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { BondEscalation storage _escalation = _escalations[_dispute.requestId]; uint256 _disputeCreatedAt = ORACLE.disputeCreatedAt(_disputeId); - if (block.timestamp <= _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) { + if (block.timestamp < _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) { revert BondEscalationModule_BondEscalationNotOver(); } @@ -302,7 +302,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { _params = decodeRequestData(_request.disputeModuleData); uint256 _disputeCreatedAt = ORACLE.disputeCreatedAt(_disputeId); - if (block.timestamp > _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) { + if (block.timestamp >= _disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer) { revert BondEscalationModule_BondEscalationOver(); } @@ -322,7 +322,7 @@ contract BondEscalationModule is Module, IBondEscalationModule { } if ( - block.timestamp > _disputeCreatedAt + _params.bondEscalationDeadline + block.timestamp >= _disputeCreatedAt + _params.bondEscalationDeadline && _numPledgersForDispute == _numPledgersAgainstDispute ) { revert BondEscalationModule_CannotBreakTieDuringTyingBuffer(); diff --git a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol index f56dd714..40286566 100644 --- a/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol +++ b/solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol @@ -85,8 +85,8 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { _escalation.startTime + _params.committingTimeWindow, _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow ); - if (block.timestamp <= _revealStartTime) revert PrivateERC20ResolutionModule_OnGoingCommittingPhase(); - if (block.timestamp > _revealEndTime) revert PrivateERC20ResolutionModule_RevealingPhaseOver(); + if (block.timestamp < _revealStartTime) revert PrivateERC20ResolutionModule_OnGoingCommittingPhase(); + if (block.timestamp >= _revealEndTime) revert PrivateERC20ResolutionModule_RevealingPhaseOver(); VoterData storage _voterData = _votersData[_disputeId][msg.sender]; @@ -123,7 +123,7 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule { if (block.timestamp < _escalation.startTime + _params.committingTimeWindow) { revert PrivateERC20ResolutionModule_OnGoingCommittingPhase(); } - if (block.timestamp <= _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) { + if (block.timestamp < _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) { revert PrivateERC20ResolutionModule_OnGoingRevealingPhase(); } diff --git a/solidity/test/integration/EscalateDispute.t.sol b/solidity/test/integration/EscalateDispute.t.sol index 569d574b..3301d31f 100644 --- a/solidity/test/integration/EscalateDispute.t.sol +++ b/solidity/test/integration/EscalateDispute.t.sol @@ -42,7 +42,7 @@ contract Integration_EscalateDispute is IntegrationBase { maxNumberOfEscalations: 1, bondEscalationDeadline: _expectedDeadline, tyingBuffer: 0, - disputeWindow: 0 + disputeWindow: _baseDisputeWindow }) ); diff --git a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol index 79effdd7..d45444d4 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationModule.t.sol @@ -237,6 +237,8 @@ contract BondEscalationModule_Unit_EscalateDispute is BaseTest { // dispute once before the bond escalation deadline is over, and that dispute goes through the escalation module. bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId); + vm.warp(disputeCreatedAt + _params.bondEscalationDeadline - 1); + // Check: does it revert if the bond escalation is not over yet? vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector); vm.prank(address(oracle)); @@ -453,7 +455,7 @@ contract BondEscalationModule_Unit_DisputeResponse is BaseTest { mockDispute.responseId = _responseId; // Warp to a time after the disputeWindow is over. - vm.warp(block.timestamp + _disputeWindow + 1); + vm.warp(block.timestamp + _disputeWindow); // Mock and expect IOracle.responseCreatedAt to be called _mockAndExpect(address(oracle), abi.encodeCall(IOracle.responseCreatedAt, (_responseId)), abi.encode(1)); @@ -1045,13 +1047,44 @@ contract BondEscalationModule_Unit_PledgeForDispute is BaseTest { _setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers); // warp into the tyingBuffer - vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + 1); + vm.warp(disputeCreatedAt + _params.bondEscalationDeadline); // Check: does it revert if trying to tie outside of the tying buffer? vm.expectRevert(IBondEscalationModule.BondEscalationModule_CannotBreakTieDuringTyingBuffer.selector); bondEscalationModule.pledgeForDispute(mockRequest, _dispute); } + /** + * @notice Tests that pledgeAgainstDispute reverts if someone tries to pledge for when the escalation is over + */ + function test_revertIfBondEscalationOver(IBondEscalationModule.RequestParameters memory _params) + public + assumeFuzzable(address(_params.accountingExtension)) + { + _params.bondSize = 1000; + _params.maxNumberOfEscalations = 3; + _params.bondEscalationDeadline = bondEscalationDeadline; + _params.tyingBuffer = 1000; + mockRequest.disputeModuleData = abi.encode(_params); + + (, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle); + bytes32 _requestId = _getId(mockRequest); + bytes32 _disputeId = _getId(_dispute); + + // Mock and expect + bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId); + + uint256 _numForPledgers = 2; + uint256 _numAgainstPledgers = _numForPledgers + 1; + + _setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers); + + vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer + 1); + + vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationOver.selector); + bondEscalationModule.pledgeForDispute(mockRequest, _dispute); + } + /** * @notice Tests that pledgeForDispute is called successfully */ @@ -1236,13 +1269,44 @@ contract BondEscalationModule_Unit_PledgeAgainstDispute is BaseTest { _setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers); - vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + 1); + vm.warp(disputeCreatedAt + _params.bondEscalationDeadline); // Check: does it revert if trying to tie outside of the tying buffer? vm.expectRevert(IBondEscalationModule.BondEscalationModule_CannotBreakTieDuringTyingBuffer.selector); bondEscalationModule.pledgeAgainstDispute(mockRequest, _dispute); } + /** + * @notice Tests that pledgeAgainstDispute reverts if someone tries to pledge against when the escalation is over + */ + function test_revertIfBondEscalationOver(IBondEscalationModule.RequestParameters memory _params) + public + assumeFuzzable(address(_params.accountingExtension)) + { + _params.bondSize = 1000; + _params.maxNumberOfEscalations = 3; + _params.bondEscalationDeadline = bondEscalationDeadline; + _params.tyingBuffer = 1000; + mockRequest.disputeModuleData = abi.encode(_params); + + (, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle); + bytes32 _requestId = _getId(mockRequest); + bytes32 _disputeId = _getId(_dispute); + + // Mock and expect + bondEscalationModule.forTest_setEscalatedDispute(_requestId, _disputeId); + + uint256 _numForPledgers = 2; + uint256 _numAgainstPledgers = _numForPledgers + 1; + + _setBondEscalation(_requestId, _numForPledgers, _numAgainstPledgers); + + vm.warp(disputeCreatedAt + _params.bondEscalationDeadline + _params.tyingBuffer + 1); + + vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationOver.selector); + bondEscalationModule.pledgeAgainstDispute(mockRequest, _dispute); + } + /** * @notice Tests that pledgeAgainstDispute is called successfully */ @@ -1331,6 +1395,8 @@ contract BondEscalationModule_Unit_SettleBondEscalation is BaseTest { address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_getId(_dispute))), abi.encode(disputeCreatedAt) ); + vm.warp(disputeCreatedAt + bondEscalationDeadline - 1); + // Check: does it revert if the bond escalation is not over? vm.expectRevert(IBondEscalationModule.BondEscalationModule_BondEscalationNotOver.selector); bondEscalationModule.settleBondEscalation(mockRequest, _response, _dispute); diff --git a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol index 09cb0530..42700039 100644 --- a/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol +++ b/solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol @@ -51,6 +51,10 @@ contract BaseTest is Test, Helpers { // A mock token IERC20 public token; + uint256 public constant REVEALING_TIME_WINDOW = 40_000; + uint256 public constant START_TIME = 100_000; + uint256 public constant END_TIME = START_TIME + REVEALING_TIME_WINDOW; + // Events event CommittingPhaseStarted(uint256 _startTime, bytes32 _disputeId); event VoteCommitted(address _voter, bytes32 _disputeId, bytes32 _commitment); @@ -95,7 +99,7 @@ contract BaseTest is Test, Helpers { ); module.commitVote(_request, _dispute, _commitment); - vm.warp(140_001); + vm.warp(END_TIME + 1); vm.mockCall( address(token), @@ -173,7 +177,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { votingToken: token, minVotesForQuorum: 1, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -183,7 +187,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { bytes32 _disputeId = _getId(mockDispute); // Store mock escalation data with startTime 100_000 - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); // Set timestamp for valid committingTimeWindow vm.warp(123_456); @@ -365,7 +369,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { * @notice Test that `commitVote` reverts if called outside of the committing time window. */ function test_revertIfCommittingPhaseOver(uint256 _timestamp, bytes32 _commitment) public { - _timestamp = bound(_timestamp, 140_000, type(uint96).max); + _timestamp = bound(_timestamp, END_TIME, type(uint96).max); // Set mock request data mockRequest.resolutionModuleData = abi.encode( @@ -374,7 +378,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { votingToken: token, minVotesForQuorum: 1, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -384,7 +388,7 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest { bytes32 _disputeId = _getId(mockDispute); // Store mock escalation data with startTime 100_000 - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); // Warp to invalid timestamp for commitment vm.warp(_timestamp); @@ -414,7 +418,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { votingToken: token, minVotesForQuorum: 1, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -427,7 +431,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_disputeId)), abi.encode(1)); // Store mock escalation data with startTime 100_000 - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); // Store commitment vm.prank(_voter); @@ -491,7 +495,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { * @notice Test that `revealVote` reverts if called outside the revealing time window. */ function test_revertIfInvalidPhase(uint256 _numberOfVotes, bytes32 _salt, uint256 _timestamp) public { - vm.assume(_timestamp >= 100_000 && (_timestamp <= 140_000 || _timestamp > 180_000)); + vm.assume(_timestamp >= 100_000 && (_timestamp < END_TIME || _timestamp > 180_000)); // Set mock request data mockRequest.resolutionModuleData = abi.encode( @@ -500,7 +504,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { votingToken: token, minVotesForQuorum: 1, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -511,12 +515,12 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, _disputeId), abi.encode(1)); - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); // Jump to timestamp vm.warp(_timestamp); - if (_timestamp <= 140_000) { + if (_timestamp < END_TIME) { // Check: does it revert if trying to reveal during the committing phase? vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_OnGoingCommittingPhase.selector); module.revealVote(mockRequest, _dispute, _numberOfVotes, _salt); @@ -550,7 +554,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { votingToken: token, minVotesForQuorum: 1, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -562,7 +566,7 @@ contract PrivateERC20ResolutionModule_Unit_RevealVote is BaseTest { // Mock and expect IOracle.disputeCreatedAt to be called _mockAndExpect(address(oracle), abi.encodeCall(IOracle.disputeCreatedAt, (_disputeId)), abi.encode(1)); - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); vm.warp(150_000); @@ -601,7 +605,7 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { votingToken: token, minVotesForQuorum: _minVotesForQuorum, committingTimeWindow: 40_000, - revealingTimeWindow: 40_000 + revealingTimeWindow: REVEALING_TIME_WINDOW }) ); @@ -613,7 +617,7 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { mockDispute.responseId = _responseId; bytes32 _disputeId = _getId(mockDispute); - module.forTest_setStartTime(_disputeId, 100_000); + module.forTest_setStartTime(_disputeId, START_TIME); // Store escalation data with startTime 100_000 and votes 0 uint256 _votersAmount = 5; @@ -701,7 +705,7 @@ contract PrivateERC20ResolutionModule_Unit_ResolveDispute is BaseTest { // Jump to timestamp vm.warp(_timestamp); - if (_timestamp <= 500_000) { + if (_timestamp < 500_000) { // Check: does it revert if trying to resolve during the committing phase? vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_OnGoingCommittingPhase.selector); vm.prank(address(oracle));