Fix: ToB-11 (dispute collusion) #1519
Conversation
- Notary in ongoing Dispute / slashed - Notary recently won a Dispute
WalkthroughThe changes primarily focus on enhancing the dispute resolution mechanism in the contracts. They introduce a new Changes
TipsChat with CodeRabbit Bot (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1519 +/- ##
===================================================
+ Coverage 50.76253% 50.81212% +0.04959%
===================================================
Files 357 357
Lines 23868 23888 +20
Branches 273 280 +7
===================================================
+ Hits 12116 12138 +22
+ Misses 10598 10596 -2
Partials 1154 1154
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
github-actions
bot
added
2-reviewers
C-Protocol-Critical
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (15)
- packages/contracts-core/contracts/Destination.sol (4 hunks)
- packages/contracts-core/contracts/Summit.sol (4 hunks)
- packages/contracts-core/contracts/base/AgentSecured.sol (5 hunks)
- packages/contracts-core/contracts/hubs/ExecutionHub.sol (2 hunks)
- packages/contracts-core/contracts/interfaces/IAgentSecured.sol (2 hunks)
- packages/contracts-core/contracts/libs/Constants.sol (1 hunks)
- packages/contracts-core/contracts/libs/Errors.sol (1 hunks)
- packages/contracts-core/contracts/libs/Structures.sol (1 hunks)
- packages/contracts-core/test/mocks/base/AgentSecuredMock.t.sol (2 hunks)
- packages/contracts-core/test/suite/Destination.t.sol (5 hunks)
- packages/contracts-core/test/suite/DestinationSynapse.t.sol (3 hunks)
- packages/contracts-core/test/suite/Summit.t.sol (2 hunks)
- packages/contracts-core/test/suite/SummitTips.t.sol (5 hunks)
- packages/contracts-core/test/suite/base/AgentSecured.t.sol (2 hunks)
- packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol (2 hunks)
Files skipped from review due to trivial changes (4)
- packages/contracts-core/contracts/libs/Constants.sol
- packages/contracts-core/contracts/libs/Errors.sol
- packages/contracts-core/contracts/libs/Structures.sol
- packages/contracts-core/test/mocks/base/AgentSecuredMock.t.sol
Additional comments: 44
packages/contracts-core/contracts/interfaces/IAgentSecured.sol (2)
1-4: The import statement for
DisputeStatusfromStructures.solis added correctly. Ensure that theStructures.solfile is in the correct path and that theDisputeStatusstruct is defined in it.57-64: The
latestDisputeStatusfunction is added to theIAgentSecuredinterface. This function is expected to return the latest dispute status for an agent given its index in the Agent Merkle Tree. Ensure that this function is implemented in the contract that uses this interface.packages/contracts-core/test/suite/Summit.t.sol (2)
1-11: The import statements look fine and are importing necessary contracts and libraries. Ensure that the imported contracts and libraries are up-to-date and compatible with the current version of solidity (0.8.17).
403-448: The newly added test cases
test_submitSnapshot_revert_notaryWonDisputeTimeoutandtest_submitSnapshot_afterNotaryDisputeTimeoutare checking the functionality of the dispute timeout for Notaries. The tests seem to be well-structured and cover the necessary scenarios. However, ensure that these tests are passing and providing the expected results.packages/contracts-core/contracts/hubs/ExecutionHub.sol (2)
9-15: The new exception
DisputeTimeoutNotOverhas been imported. This is used to handle the case where a notary is still in the post-dispute timeout period.368-374: The function
_proveAttestationhas been updated to check if a notary is in post-dispute timeout using the function_notaryDisputeTimeout. If the notary is in post-dispute timeout, the function reverts with theDisputeTimeoutNotOvererror. This is a good practice as it ensures that notaries who have recently been in a dispute cannot submit attestations until the timeout period is over, which can help prevent malicious behavior.+ // Check that Notary who submitted the attestation isn't in post-dispute timeout + if (_notaryDisputeTimeout(rootData.notaryIndex)) revert DisputeTimeoutNotOver();packages/contracts-core/test/suite/DestinationSynapse.t.sol (6)
1-8: The import statements look fine. Ensure that the
DISPUTE_TIMEOUT_NOTARYconstant andDisputeTimeoutNotOvererror are correctly defined in their respective files.194-225: The
prepareGasDataDisputeTestfunction seems to be setting up a test scenario for a dispute. It's important to ensure that theopenTestDisputefunction correctly simulates a dispute scenario.227-231: The
prepareNotaryWonDisputeTestfunction seems to be setting up a test scenario where a notary wins a dispute. It's important to ensure that theresolveTestDisputefunction correctly simulates a dispute resolution scenario.194-238: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [233-243]
The
test_getGasData_notaryInDisputefunction tests thegetGasDatafunction when a notary is in dispute. It's important to ensure that thegetGasDatafunction correctly handles the case when a notary is in dispute.
245-256: The
test_getGasData_notaryWonDisputeTimeoutfunction tests thegetGasDatafunction when a notary has won a dispute but is still within the dispute timeout period. It's important to ensure that thegetGasDatafunction correctly handles the case when a notary is in the dispute timeout period.259-271: The
test_getGasData_afterNotaryDisputeTimeoutfunction tests thegetGasDatafunction after the dispute timeout period for a notary has passed. It's important to ensure that thegetGasDatafunction correctly handles the case when the dispute timeout period for a notary has passed.packages/contracts-core/contracts/Summit.sol (4)
5-17: The import statements are well organized and the new error
DisputeTimeoutNotOveris imported correctly.118-123: The function
acceptReceiptnow checks if a notary is in dispute or if the dispute timeout is over before proceeding. This is a good security measure to ensure that only trusted notaries can submit receipts.+ if (_notaryDisputeExists(rcptNotaryIndex)) revert NotaryInDispute(); + if (_notaryDisputeTimeout(rcptNotaryIndex)) revert DisputeTimeoutNotOver();
- 146-151: The function
acceptNotarySnapshotnow checks if a notary is in dispute or if the dispute timeout is over before proceeding. This is a good security measure to ensure that only trusted notaries can submit snapshots.+ if (_notaryDisputeExists(notaryIndex)) revert NotaryInDispute(); + if (_notaryDisputeTimeout(notaryIndex)) revert DisputeTimeoutNotOver();
- 221-237: The function
_checkNotaryDisputedhas been updated to handle the case where a notary has recently won a dispute and is in the post-dispute timeout period. This is a good way to ensure that the notary's statements are not accepted during the timeout period.+ } else if (flag == DisputeFlag.Pending || _notaryDisputeTimeout(notaryIndex)) { + // Notary is in the ongoing Dispute, or has recently won one. We postpone the receipt handling. + // To keep the tips flow going we add the receipt to the back of the queue, + // hoping that by the next interaction the dispute will have been resolved. + _moveToBack(); + queuePopped = true; + }packages/contracts-core/test/suite/base/AgentSecured.t.sol (9)
1-13: The import statements and the
updateOriginfunction look fine. No issues found.22-25: The
openTestDisputefunction is a helper function for testing. It impersonates thelocalAgentManagerand opens a dispute. This is a good practice for testing purposes.27-30: The
resolveTestDisputefunction is another helper function for testing. It impersonates thelocalAgentManagerand resolves a dispute. This is a good practice for testing purposes.32-37: The
checkLatestDisputeStatusfunction checks if the latest dispute status matches the expected status. This is a good practice for testing purposes.39-45: The
test_openDisputefunction tests theopenDisputefunction. It sets the expected dispute status, opens a dispute, and checks if the dispute status matches the expected status. This is a good practice for testing purposes.47-52: The
test_openDispute_revert_onlyAgentManagerfunction tests if theopenDisputefunction reverts when called by someone other than thelocalAgentManager. This is a good practice for testing purposes.56-63: The
test_resolveDisputefunction tests theresolveDisputefunction. It opens a dispute, resolves it, and checks if the dispute status matches the expected status. This is a good practice for testing purposes.65-71: The
test_resolveDispute_revert_onlyAgentManagerfunction tests if theresolveDisputefunction reverts when called by someone other than thelocalAgentManager. This is a good practice for testing purposes.75-78: The
localAgentSecuredfunction returns the tested contract asIAgentSecured. This is a good practice for testing purposes.packages/contracts-core/contracts/base/AgentSecured.sol (7)
2-11: The import statements look good and are importing the necessary libraries and interfaces for the contract.
34-38: The
_disputesmapping is correctly defined to store the dispute status of agents. TheDisputeStatusstruct is used to store the flag, openedAt timestamp, and resolvedAt timestamp for each agent.58-65: The
openDisputefunction correctly sets the dispute status for the specified guard and notary agents. It uses theChainContext.blockTimestamp()function to get the current timestamp and sets theDisputeFlagtoPending.68-78: The
resolveDisputefunction correctly resolves the dispute for the slashed agent and, if specified, marks the rival agent as not disputed. It uses theChainContext.blockTimestamp()function to get the current timestamp and sets theDisputeFlagtoSlashedfor the slashed agent andNonefor the rival agent.92-95: The
latestDisputeStatusfunction correctly retrieves the dispute status of an agent from the_disputesmapping.109-116: The
_notaryDisputeExistsfunction correctly checks if a notary agent is in an ongoing dispute or has been slashed by checking if theDisputeFlagis notNone.118-131: The
_notaryDisputeTimeoutfunction correctly checks if a notary agent is in the post-dispute timeout period. It first checks if the notary is in an ongoing dispute or has been slashed, then checks if the notary has been in any dispute at all, and finally checks if the dispute timeout is still active by comparing the current timestamp with the sum of theresolvedAttimestamp andDISPUTE_TIMEOUT_NOTARY.packages/contracts-core/test/suite/Destination.t.sol (7)
4-5: The import statements have been updated to include
DISPUTE_TIMEOUT_NOTARYfromConstants.solandDisputeTimeoutNotOverfromErrors.sol. Ensure that these new constants and errors are correctly defined in their respective files.198-214: The
test_acceptAttestation_revert_notaryWonDisputeTimeoutfunction tests the scenario where a notary has won a dispute but the dispute timeout is not over. It checks that thesubmitAttestationfunction reverts with theDisputeTimeoutNotOvererror. This is a good test case to ensure that the dispute timeout is being enforced correctly.216-236: The
test_acceptAttestation_afterNotaryDisputeTimeoutfunction tests the scenario where a notary has won a dispute and the dispute timeout is over. It checks that thesubmitAttestationfunction does not revert and that the attestation nonce is updated. This is a good test case to ensure that attestations are accepted after the dispute timeout.340-356: The
prepareAgentRootDisputeTesthelper function prepares a mock attestation and opens a dispute for the Notary that signed it. This is a useful helper function for setting up test scenarios involving disputes.358-363: The
prepareNotaryWonDisputeTesthelper function resolves a test dispute in favor of the Notary. This is a useful helper function for setting up test scenarios where the Notary has won a dispute.365-456: These test functions test the
passAgentRootfunction under various scenarios involving disputes and dispute timeouts. They check that the agent root is passed or not passed correctly depending on the dispute status and timeout. These are good test cases to ensure that thepassAgentRootfunction behaves correctly in these scenarios.546-595: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [549-614]
These test functions test the
getGasDatafunction under various scenarios involving disputes and dispute timeouts. They check that the gas data is returned or not returned correctly depending on the dispute status and timeout. These are good test cases to ensure that thegetGasDatafunction behaves correctly in these scenarios.packages/contracts-core/test/suite/SummitTips.t.sol (5)
4-11: The import of
DISPUTE_TIMEOUT_NOTARYandDisputeTimeoutNotOverindicates that the dispute timeout functionality is being tested in this suite.130-141: The
prepareNotaryInDisputeTestandprepareNotaryWonDisputeTestfunctions are helper functions that simulate a notary being in a dispute and a notary winning a dispute, respectively. These will be useful for testing the new dispute timeout functionality.201-216: The
test_submitReceipt_revert_signedByGuardandtest_submitReceipt_revert_notaryInDisputefunctions are testing that thesubmitReceiptfunction correctly rejects receipts signed by a guard or a notary in dispute. This is good for ensuring the security and correctness of thesubmitReceiptfunction.344-367: The
test_distributeTips_attestationNotary_wonDisputeTimeoutandtest_distributeTips_attestationNotary_afterNotaryDisputeTimeoutfunctions are testing the new dispute timeout functionality. They ensure that a notary who has won a dispute cannot distribute tips until the dispute timeout period is over. This is good for ensuring the correctness of the dispute timeout functionality.409-432: The
test_distributeTips_receiptNotary_wonDisputeTimeoutandtest_distributeTips_receiptNotary_afterNotaryDisputeTimeoutfunctions are also testing the new dispute timeout functionality. They ensure that a notary who has won a dispute cannot distribute tips until the dispute timeout period is over. This is good for ensuring the correctness of the dispute timeout functionality.
There was a problem hiding this comment.
lgtm as is.
would say tob 11 in point 2 of recommendations suggested a new status for agents becoming active after dispute resolution (in post dispute timeout), so you may want to add that as well. but current checks in this PR effectively implement that functionality so your call.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/contracts-core/contracts/libs/Constants.sol (1 hunks)
- packages/contracts-core/contracts/libs/Errors.sol (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/contracts-core/contracts/libs/Constants.sol
- packages/contracts-core/contracts/libs/Errors.sol
Description
SummitDestinationLightManagerSummary by CodeRabbit
New Features:
Refactor:
Tests:
Chores:
Bug Fixes: