Fix: Zellic-3.3 (safety checks missing) #1462

ChiTimesChi · 2023-10-18T18:29:02Z

Description

Fixed Zellic 3.3 by adding the origin domain check in BondingManager.remoteSlashAgent() and Destination.execute()
Also added a check to ensure message for the local domain could not be sent using Origin

Summary by CodeRabbit

New Feature: Introduced safeguards to prevent messages from being sent or executed within the same domain, enhancing the security and reliability of the system.
Test: Added comprehensive tests to ensure the correct functioning of these new features and to prevent messages from being sent or executed within the same domain.
Documentation: Updated function documentation to reflect new conditions under which functions will revert, improving clarity for developers interacting with the system.

- remoteSlashAgent from the same domain

coderabbitai · 2023-10-18T18:29:08Z

Walkthrough

The changes primarily focus on introducing new checks to prevent messages from being sent or executed within the same domain. This is achieved by adding new exceptions, IncorrectOriginDomain and IncorrectDestinationDomain, and modifying functions in various contracts to include these checks. New test cases have been added to verify these changes.

Changes

File(s)	Summary
`.../contracts/manager/BondingManager.sol`, `.../test/suite/manager/BondingManager.t.sol`	Introduced `IncorrectOriginDomain` exception and added checks to prevent slashing an agent if the message origin is the local domain. Modified and added test cases accordingly.
`.../contracts/hubs/ExecutionHub.sol`, `.../test/suite/hubs/ExecutionHub.t.sol`	Added a check to prevent execution of messages originating from the local domain. Added a test case to verify this change.
`.../contracts/Origin.sol`, `.../contracts/interfaces/IExecutionHub.sol`, `.../contracts/interfaces/InterfaceBondingManager.sol`, `.../contracts/interfaces/InterfaceOrigin.sol`, `.../test/suite/Origin.t.sol`	Introduced `IncorrectDestinationDomain` error and added checks to prevent sending messages to the local domain. Updated function documentation and added test cases to verify these changes.

🐇
"In the land of code, where the shadows lie,
A rabbit hopped, with a watchful eye.
No message now, shall in its domain stay,
For the rabbit has found a better way.
With a hop and a skip, and a joyful heart,
The rabbit celebrates this fresh start." 🎉

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

Mention @coderabbitai in any review comment for bot assistance.
Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
resume reviews, enter @coderabbitai resume as any issue or review comment.

coderabbitai

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between dcda614 and 34a067a.

Files selected for processing (2)

packages/contracts-core/contracts/manager/BondingManager.sol (2 hunks)
packages/contracts-core/test/suite/manager/BondingManager.t.sol (2 hunks)

Additional comments (Suppressed): 5

packages/contracts-core/contracts/manager/BondingManager.sol (2)
11-11: The new error IncorrectOriginDomain has been added. Ensure that this error is properly handled in all places where it can be thrown.

172-178: The check for the origin of the message has been added. This is a security enhancement that prevents an agent from being slashed from the local domain. The change looks good, but make sure to update any relevant documentation or comments to reflect this new behavior.
-        // TODO: do we need to save this?
-        msgOrigin;
+        // Message needs to be sent from the remote chain
+        if (msgOrigin == localDomain) revert IncorrectOriginDomain();
packages/contracts-core/test/suite/manager/BondingManager.t.sol (3)

9-15: The new error IncorrectOriginDomain has been imported from Errors.sol. Ensure that this error is defined and implemented correctly in Errors.sol.

252-258: A new test test_remoteSlashAgent_revert_sameOriginDomain has been added. This test checks if the IncorrectOriginDomain error is thrown when the origin of a message is the local domain. The test seems to be correctly implemented.

260-262: The function test_completeSlashing_active has been modified. Ensure that the changes in the function signature are reflected wherever this function is called.

codecov · 2023-10-18T18:32:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (04ba913) 50.44029% compared to head (4e3d8d0) 50.44437%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@                 Coverage Diff                 @@
##              master       #1462         +/-   ##
===================================================
+ Coverage   50.44029%   50.44437%   +0.00408%     
===================================================
  Files            356         356                 
  Lines          24302       24304          +2     
  Branches         269         271          +2     
===================================================
+ Hits           12258       12260          +2     
  Misses         10863       10863                 
  Partials        1181        1181

Flag	Coverage Δ
packages	`90.62003% <ø> (ø)`
solidity	`93.04770% <100.00000%> (+0.01126%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/contracts-core/contracts/Origin.sol	`95.45455% <ø> (ø)`
...ges/contracts-core/contracts/hubs/ExecutionHub.sol	`97.56098% <100.00000%> (+0.03011%)`	⬆️
...ontracts-core/contracts/manager/BondingManager.sol	`86.74699% <100.00000%> (+0.16161%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- `execute` sent from the same domain

coderabbitai

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 34a067a and e55f5f0.

Files selected for processing (2)

packages/contracts-core/contracts/hubs/ExecutionHub.sol (2 hunks)
packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol (2 hunks)

Additional comments (Suppressed): 4

packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol (2)

7-13: The new exception IncorrectOriginDomain has been added to the list of exceptions. This is in line with the changes described in the PR summary.

330-343: A new test case test_execute_base_revert_originSameDomain has been added. This test case checks if the contract correctly throws the IncorrectOriginDomain exception when the origin and destination domains are the same. The test case seems to be correctly set up and should work as expected.
packages/contracts-core/contracts/hubs/ExecutionHub.sol (2)
15-18: The new exception IncorrectOriginDomain has been added to handle cases where the origin and destination domains are the same. This is a good addition for safety checks.

122-129: The check for the origin of the message has been added. If the origin is the same as the local domain, the function will revert with the IncorrectOriginDomain error. This is a good security measure to prevent execution of messages originating from the local domain.
+         // Ensure message was not sent from this domain
+         if (header.origin() == localDomain) revert IncorrectOriginDomain();

- Sending base message to the same domain - Sending manager message to the same domain

coderabbitai

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between e55f5f0 and ed0d6f0.

Files selected for processing (5)

packages/contracts-core/contracts/Origin.sol (4 hunks)
packages/contracts-core/contracts/interfaces/IExecutionHub.sol (1 hunks)
packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol (1 hunks)
packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol (2 hunks)
packages/contracts-core/test/suite/Origin.t.sol (2 hunks)

Files skipped from review due to trivial changes (2)

packages/contracts-core/contracts/interfaces/IExecutionHub.sol
packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol

Additional comments (Suppressed): 9

packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol (2)

7-16: The new conditions for revert in the sendBaseMessage function are well documented. Ensure that these conditions are properly handled in the implementation of this function and that all calls to this function throughout the codebase are updated to match these conditions.

33-39: The new revert condition in the sendManagerMessage function is well documented. Ensure that this condition is properly handled in the implementation of this function and that all calls to this function throughout the codebase are updated to match this condition.

packages/contracts-core/contracts/Origin.sol (4)

4-15: The new hunk introduces the IncorrectDestinationDomain error from the Errors.sol library. This error is used to handle cases where the destination domain is the local domain, which is not allowed in the updated contract. This is a good addition for error handling and improves the robustness of the contract.

40-46: The onlyRemoteDestination modifier is introduced to check if the destination domain is not the local domain. If it is, it reverts the transaction with an IncorrectDestinationDomain error. This is a good practice to encapsulate this logic in a modifier for reusability and readability.

73-79: The sendMessage function now includes the onlyRemoteDestination modifier to ensure that the destination is not the local domain. This is a good practice to prevent messages from being sent to the local domain. However, ensure that all calls to this function throughout the codebase have been updated to match the new function signature.

95-101: The sendManagerMessage function now includes the onlyRemoteDestination modifier to ensure that the destination is not the local domain. This is a good practice to prevent manager messages from being sent to the local domain. However, ensure that all calls to this function throughout the codebase have been updated to match the new function signature.

packages/contracts-core/test/suite/Origin.t.sol (3)

4-15: The new hunk introduces the IncorrectDestinationDomain error from the Errors.sol library. This error is used in the new test cases to verify that the sendBaseMessage and sendManagerMessage functions in the Origin contract correctly prevent messages from being sent to the local domain.

106-112: This new test case, test_sendBaseMessage_revert_sameDestination, checks if the sendBaseMessage function in the Origin contract correctly throws the IncorrectDestinationDomain error when the destination is the local domain. The vm.expectRevert function is used to expect the IncorrectDestinationDomain error, and the vm.prank function is used to set the sender of the transaction.

114-118: This new test case, test_sendManagementMessage_revert_sameDestination, checks if the sendManagerMessage function in the Origin contract correctly throws the IncorrectDestinationDomain error when the destination is the local domain. The vm.expectRevert function is used to expect the IncorrectDestinationDomain error, and the vm.prank function is used to set the sender of the transaction.

ChiTimesChi added 2 commits October 18, 2023 19:26

Add test with the expected revert:

0750988

- remoteSlashAgent from the same domain

Fix: check origin domain

34a067a

ChiTimesChi requested a review from trajan0x as a code owner October 18, 2023 18:29

github-actions bot added M-contracts Module: Contracts 2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. size/xs labels Oct 18, 2023

coderabbitai bot reviewed Oct 18, 2023

View reviewed changes

ChiTimesChi added 2 commits October 18, 2023 19:42

Add test with the expected revert:

4527220

- `execute` sent from the same domain

Fix: check origin domain in ExecutionHub

e55f5f0

coderabbitai bot reviewed Oct 18, 2023

View reviewed changes

github-actions bot added the needs-go-generate-agents label Oct 18, 2023

ChiTimesChi added 3 commits October 18, 2023 19:54

Add tests with the expected reverts:

cb4398a

- Sending base message to the same domain - Sending manager message to the same domain

Fix: revert when message destination is local domain

5448b42

Update "will revert" documentation

ed0d6f0

github-actions bot added size/s and removed size/xs needs-go-generate-agents labels Oct 18, 2023

coderabbitai bot reviewed Oct 18, 2023

View reviewed changes

trajan0x approved these changes Oct 20, 2023

View reviewed changes

Merge branch 'master' into fix/zellic/3.3-safety-checks-missing

4e3d8d0

github-actions bot added the needs-go-generate-agents label Oct 23, 2023

ChiTimesChi merged commit cdcd992 into master Oct 23, 2023
53 checks passed

ChiTimesChi deleted the fix/zellic/3.3-safety-checks-missing branch October 23, 2023 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Zellic-3.3 (safety checks missing) #1462

Fix: Zellic-3.3 (safety checks missing) #1462

ChiTimesChi commented Oct 18, 2023 •

edited

Loading

coderabbitai bot commented Oct 18, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

Pause Incremental Reviews

coderabbitai bot left a comment

codecov bot commented Oct 18, 2023 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

Fix: Zellic-3.3 (safety checks missing) #1462

Fix: Zellic-3.3 (safety checks missing) #1462

Conversation

ChiTimesChi commented Oct 18, 2023 • edited Loading

Summary by CodeRabbit

coderabbitai bot commented Oct 18, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

Pause Incremental Reviews

coderabbitai bot left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

ChiTimesChi commented Oct 18, 2023 •

edited

Loading

coderabbitai bot commented Oct 18, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

codecov bot commented Oct 18, 2023 •

edited

Loading