fix(protocol)!: add message owner parameter to vault operations #15770
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Brechtpd
changed the title
dantaik
approved these changes
Feb 13, 2024
|
@KorbinianK @cyberhorsey @xiaodino please also review this PR as there is a minor breaking change that bridge shall be aware of. |
|
Oh was double checking the owner use, and recall still uses |
|
Fixed, but please also double check that |
Brechtpd
changed the title
dantaik
approved these changes
Feb 14, 2024
Doubled checked, looks good. |
dantaik
changed the title
WalkthroughThe recent updates focus on enhancing clarity and flexibility in handling ownership across blockchain bridges. By differentiating between Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
adaki2004
approved these changes
Feb 14, 2024
![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: 4
Configuration used: CodeRabbit UI
Files selected for processing (12)
- packages/protocol/contracts/L1/libs/LibProving.sol (2 hunks)
- packages/protocol/contracts/bridge/Bridge.sol (10 hunks)
- packages/protocol/contracts/bridge/IBridge.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BaseNFTVault.sol (1 hunks)
- packages/protocol/contracts/tokenvault/ERC1155Vault.sol (3 hunks)
- packages/protocol/contracts/tokenvault/ERC20Vault.sol (4 hunks)
- packages/protocol/contracts/tokenvault/ERC721Vault.sol (3 hunks)
- packages/protocol/genesis/GenerateGenesis.g.sol (2 hunks)
- packages/protocol/test/bridge/Bridge.t.sol (8 hunks)
- packages/protocol/test/tokenvault/ERC1155Vault.t.sol (14 hunks)
- packages/protocol/test/tokenvault/ERC20Vault.t.sol (7 hunks)
- packages/protocol/test/tokenvault/ERC721Vault.t.sol (14 hunks)
Additional comments: 65
packages/protocol/contracts/tokenvault/BaseNFTVault.sol (1)
- 38-39: The addition of
destOwnerin theBridgeTransferOpstruct aligns with the PR objectives to differentiate between source and destination owners. Ensure that this field is utilized correctly in all relevant operations.packages/protocol/contracts/tokenvault/ERC721Vault.sol (4)
- 58-59: Correctly replaced
message.ownerwithmessage.srcOwnerand addedmessage.destOwner. This change aligns with the PR objectives for distinguishing between source and destination owners.- 73-73: Ensure that the
TokenSentevent correctly logsmessage.srcOwneras the sender. This change is consistent with the updated logic for handling source and destination owners.- 137-138: Correctly using
message.srcOwnerfor token transfers back to the owner in theonMessageRecalledfunction. This aligns with the PR's objective of correctly identifying the source owner.- 142-142: The
TokenReleasedevent correctly logsmessage.srcOwneras the sender, aligning with the updated logic for handling source and destination owners.packages/protocol/contracts/tokenvault/ERC1155Vault.sol (4)
- 68-69: Correctly replaced
message.ownerwithmessage.srcOwnerand addedmessage.destOwner. This change aligns with the PR objectives for distinguishing between source and destination owners.- 85-85: Ensure that the
TokenSentevent correctly logsmessage.srcOwneras the sender. This change is consistent with the updated logic for handling source and destination owners.- 156-157: Correctly using
message.srcOwnerfor token transfers back to the owner in theonMessageRecalledfunction. This aligns with the PR's objective of correctly identifying the source owner.- 162-162: The
TokenReleasedevent correctly logsmessage.srcOwneras the sender, aligning with the updated logic for handling source and destination owners.packages/protocol/genesis/GenerateGenesis.g.sol (2)
- 163-164: The addition of
srcOwneranddestOwnerin thebridgeProxy.processMessagecall aligns with the PR objectives. Ensure that these fields are utilized correctly in all relevant operations within the contract.- 189-190: The addition of
srcOwneranddestOwnerin thebridgeProxy.processMessagecall within theexpectRevertblock aligns with the PR objectives. Ensure that these fields are utilized correctly in all relevant operations within the contract.packages/protocol/contracts/tokenvault/ERC20Vault.sol (5)
- 43-43: Addition of
destOwnerinBridgeTransferOpstruct aligns with the PR objective to differentiate between source and destination owners.- 190-191: The logic to default
destOwnertomsg.senderif not provided is correctly implemented, ensuring backward compatibility and flexibility.- 205-205: Event
TokenSentcorrectly includes_message.srcOwnerto reflect the source owner in event emissions, aligning with the changes made.- 267-268: In
onMessageRecalled, usingmessage.srcOwnerfor token transfer ensures that the tokens are returned to the correct source owner upon message recall.- 272-272: The
TokenReleasedevent correctly logsmessage.srcOwneras the source of the token release, maintaining consistency with the updated logic.packages/protocol/test/tokenvault/ERC20Vault.t.sol (7)
- 179-179: The addition of
address(0)asdestOwnerinBridgeTransferOpfunction call correctly reflects the contract changes in tests.- 195-195: Including
address(0)asdestOwnerin this test case is consistent with the contract modifications, ensuring the tests remain valid.- 215-215: The inclusion of
address(0)asdestOwnerin theBridgeTransferOpfunction call is correctly applied, aligning with the updated contract logic.- 232-232: Correctly adding
address(0)asdestOwnerin theBridgeTransferOpfunction call for this test case reflects the necessary adjustments for the contract changes.- 258-258: The inclusion of
address(0)asdestOwnerin theBridgeTransferOpfunction call is appropriate, ensuring the test aligns with the contract's updated logic.- 270-272: Adding
address(0)asdestOwnerin theBridgeTransferOpfunction call is correctly implemented, reflecting the contract changes in the test.- 453-453: Including
address(0)asdestOwnerin theBridgeTransferOpfunction call for this test case is consistent with the contract modifications, ensuring the test remains valid.packages/protocol/contracts/L1/libs/LibProving.sol (1)
- 156-156: Separating
msgSenderto allow the prover to be any address in the future enhances flexibility in proof verification, aligning with potential protocol evolution.packages/protocol/contracts/bridge/Bridge.sol (7)
- 140-142: Ensure validation for
srcOwneranddestOwnerbeing non-zero addresses is correctly implemented to prevent operations from uninitialized addresses.- 243-243: Clarification needed: The comment suggests any address can call
processMessage, but the logic restricts certain operations todestOwner. Ensure this is intentional and correctly implemented.- 278-278: Using
msg.senderaspreferredExecutorwhengasLimitis 0 might not align with the intended logic, especially ifdestOwneris expected to be the executor in such cases. Review this logic for consistency.- 293-293: The condition to allow only
destOwnerto process the message whengasLimitis 0 is correctly implemented, aligning with the PR objectives.- 323-323: Correctly determining the refund recipient based on
message.refundTobeing zero or not aligns with expected functionality.- 344-344: The comment correctly indicates that
retryMessagecan be called by any address, includingdestOwner, aligning with the PR objectives.- 360-362: The logic to enforce
destOwneras the caller for specific conditions is correctly implemented, ensuring secure operation as per PR objectives.packages/protocol/test/bridge/Bridge.t.sol (8)
- 132-133: Correctly updating test cases to use
srcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.- 170-171: Correctly updating test cases to use
srcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.- 227-228: Correctly updating test cases to use
srcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.- 279-280: Correctly updating test cases to use
srcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.- 317-318: Correctly updating test cases to use
srcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.- 594-599: Correctly using
destOwnerfor retrying messages aligns with the PR objectives, ensuring only the rightful owner can perform retries under specific conditions.- 666-667: The test setup correctly reflects the changes made to the
IBridge.Messagestruct, ensuring tests are aligned with the updated contract logic.- 696-697: Correctly updating helper function
newMessageto usesrcOwneranddestOwnerinstead of a singleowneraligns with the PR objectives.packages/protocol/test/tokenvault/ERC721Vault.t.sol (12)
- 248-248: The
address(0)parameter is used as a placeholder forsrcOwnerordestOwner. Ensure this aligns with the intended logic of differentiating between source and destination owners in cross-chain operations. Ifaddress(0)is meant to represent a non-existent or default owner, consider documenting this explicitly in the code to avoid confusion.- 277-286: The use of
address(0)for both the token address andsrcOwner/destOwnerin a test case designed to fail due to an invalid token address is appropriate. However, ensure that the test case's intent is clearly documented, especially the reason for usingaddress(0)as the token address, to maintain readability and understandability of the test suite.- 306-306: Using
address(0)as a placeholder forsrcOwnerordestOwnerin a test case designed to fail due to an invalid amount is consistent with the PR's objectives. However, ensure that the test case's purpose and the use ofaddress(0)are well-documented for clarity.- 338-338: The addition of
address(0)in this context seems to follow the pattern established in previous changes. Ensure that its use is consistent with the intended logic for handlingsrcOwneranddestOwnerand that any assumptions or implications of usingaddress(0)are clearly documented.- 391-391: The use of
address(0)here follows the established pattern. Confirm that this is the intended usage for differentiating betweensrcOwneranddestOwnerin the context of this test case, and ensure that the rationale behind usingaddress(0)is documented.- 439-439: Consistency in using
address(0)forsrcOwnerordestOwneris observed. As before, ensure that the use ofaddress(0)is intentional and documented, especially in the context of this test case which seems to simulate a second bridging operation.- 482-482: The addition of
address(0)in this test case, which involves sending tokens along with ether, is noted. Verify thataddress(0)is used correctly as per the PR's objectives and document its use to maintain clarity in the test suite.- 541-541: Using
address(0)in the context of a message recall test case is consistent with previous changes. Ensure that this usage aligns with the intended differentiation betweensrcOwneranddestOwnerand that the choice ofaddress(0)is documented.- 582-582: The use of
address(0)in a test case involving multiple tokens is observed. Confirm that this usage is consistent with the PR's objectives and ensure that the rationale behind usingaddress(0)is clearly documented.- 637-637: The addition of
address(0)in this test case, which seems to simulate a bridging operation where the owner changes, is noted. Ensure that the use ofaddress(0)is intentional and aligns with the PR's objectives, and document its purpose.- 736-736: Using
address(0)in a test case designed to simulate a scenario where the original owner cannot claim the bridged token if sold is appropriate. Confirm that this aligns with the intended logic and document the use ofaddress(0)for clarity.- 820-820: The use of
address(0)in a test case for upgrading bridged tokens is consistent with the pattern established in previous changes. Ensure that this usage is intentional and documented, especially in the context of testing contract upgrades.packages/protocol/test/tokenvault/ERC1155Vault.t.sol (14)
- 240-249: The
BridgeTransferOpstruct initialization correctly updates parameters to reflect the intended changes. However, ensure that thedestChainIdandAliceassrcOwneranddestOwneralign with the PR objectives of differentiating between source and destination owners.- 272-281: The
BridgeTransferOpstruct initialization withaddress(0)as the token address correctly simulates an invalid token scenario. This is a good test case for ensuring the contract handles invalid token addresses as expected.- 302-311: The
BridgeTransferOpstruct initialization withamounts[0] = 0correctly simulates a scenario with zero tokens being transferred. This test case is important for ensuring the contract correctly handles attempts to transfer zero tokens.- 335-344: The
BridgeTransferOpstruct initialization in this test case is similar to the first one reviewed. It's used to test receiving tokens from a newly deployed bridged contract on the destination chain. Ensure that the test setup and assertions accurately reflect the scenario being tested, including the correct handling ofdestChainIdand ownership parameters.- 399-408: This
BridgeTransferOpstruct initialization is part of a test case for receiving tokens but not deploying a new bridged contract if it's the second time tokens are bridged. The setup appears correct for the intended test scenario. Ensure that the test assertions validate the expected behavior, especially regarding the non-deployment of a new contract on subsequent token transfers.- 452-461: The
BridgeTransferOpstruct initialization here is used to test bridging tokens back but with a different owner. Ensure that the test case accurately reflects scenarios where ownership might change on the destination chain and that the contract logic correctly handles such cases.- 506-515: The
BridgeTransferOpstruct initialization includesDavidas the destination owner, testing the scenario where tokens and ether are sent to a different owner on the destination chain. This test case is important for ensuring that the contract correctly handles transfers involving ether and changes in ownership.- 569-578: The
BridgeTransferOpstruct initialization is part of a test case for theonMessageRecalledfunctionality. Ensure that the test setup and assertions accurately reflect the scenario being tested, including the correct handling of token returns upon message recall.- 613-622: This
BridgeTransferOpstruct initialization is used to test receiving multiple ERC1155 tokens. Ensure that the test case accurately reflects scenarios of multiple token transfers and that the contract logic and assertions validate the expected behavior.- 678-687: Similar to a previous comment, this
BridgeTransferOpstruct initialization tests bridging back tokens when the owner has changed. Ensure that the test case and assertions accurately reflect and validate the intended behavior, especially regarding ownership changes on the destination chain.- 737-746: The
BridgeTransferOpstruct initialization here tests bridging back tokens with a different owner, specifically focusing on the scenario where Bob, as the new owner, bridges the tokens back. Ensure that the test case accurately reflects this scenario and that the contract logic correctly handles such ownership changes.- 792-801: This
BridgeTransferOpstruct initialization is part of a test case for ensuring that the original owner cannot claim tokens anymore if they have been sold on the destination chain. This is an important test for validating ownership logic and transfer restrictions.- 851-860: The
BridgeTransferOpstruct initialization here attempts to bridge back tokens under the original owner's name after they've been sold, expecting a revert. This test case is crucial for ensuring the contract enforces ownership correctly and prevents unauthorized token claims.- 882-891: The
BridgeTransferOpstruct initialization is part of a test case for upgrading bridged tokens. Ensure that the test setup, including the contract upgrade process, accurately reflects the scenario being tested and that the assertions validate the upgrade's success.
| @@ -227,7 +229,7 @@ contract Bridge is EssentialContract, IBridge { | |||
| // Reset the context after the message call | |||
| _resetContext(); | |||
| } else { | |||
| message.owner.sendEther(message.value); | |||
| message.srcOwner.sendEther(message.value); | |||
There was a problem hiding this comment.
Directly sending Ether using sendEther without checking the success of the operation can lead to silent failures. Consider using a safer pattern like call with proper error handling.
| @@ -312,7 +310,7 @@ | |||
| } else { | |||
| // Use the specified message gas limit if called by the owner, else | |||
| // use remaining gas | |||
| uint256 gasLimit = msg.sender == message.owner ? gasleft() : message.gasLimit; | |||
| uint256 gasLimit = msg.sender == message.destOwner ? gasleft() : message.gasLimit; | |||
There was a problem hiding this comment.
The logic to determine gasLimit based on the caller being destOwner or not seems inverted. It should use the specified gasLimit if msg.sender is destOwner, otherwise use gasleft().
- uint256 gasLimit = msg.sender == message.destOwner ? gasleft() : message.gasLimit;
+ uint256 gasLimit = msg.sender == message.destOwner ? message.gasLimit : gasleft();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| uint256 gasLimit = msg.sender == message.destOwner ? gasleft() : message.gasLimit; | |
| uint256 gasLimit = msg.sender == message.destOwner ? message.gasLimit : gasleft(); |
Comment on lines
+30
to
+33
| // The owner of the message on the source chain. | ||
| address srcOwner; | ||
| // The owner of the message on the destination chain. | ||
| address destOwner; |
There was a problem hiding this comment.
Ensure the new srcOwner and destOwner fields are properly documented to clarify their roles in cross-chain operations.
| // The destination address on the destination chain. | ||
| address to; | ||
| // Alternate address to send any refund. If blank, defaults to owner. | ||
| // Alternate address to send any refund on the destination chain. | ||
| // If blank, defaults to destOwner. |
There was a problem hiding this comment.
Clarify in the comment for refundTo that it refers to an address on the destination chain, enhancing understanding for future developers.
This was referenced Apr 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #15740 (comment)
There's now a separate owner on both the source (
srcOwner) and destination chain (destOwner). This is necessary because thesrcOwner(msg.sender) that does the bridge operation on the source chain may not own the same address on the destination chain. That's wheredestOwnercomes in, that's an address on the destination chain that thesrcOwneralso needs to control. So we have to be careful to only usesrcOwneron the source chain anddestChainon the destination chain to make sure the message owner actually does have access to the message on both chains as expected.Please also double check that srcOwner is only used on the source chain, and destOwner is only used on the destination chain.
Summary by CodeRabbit
srcOwneranddestOwner.