fix: fix minor issues found in audit #82
Conversation
WalkthroughThe recent changes across multiple contracts enhance Ether handling, improve function efficiency, and introduce new functionalities. Key adjustments include checks against non-zero Ether transfers, optimizations using Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…non beacon chain eth
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- src/core/Bootstrap.sol (7 hunks)
- src/core/ClientChainGateway.sol (1 hunks)
- src/core/CustomProxyAdmin.sol (1 hunks)
- src/core/ExoCapsule.sol (2 hunks)
- src/core/ExocoreGateway.sol (1 hunks)
- src/core/NativeRestakingController.sol (1 hunks)
- src/interfaces/IExoCapsule.sol (1 hunks)
- src/libraries/Errors.sol (1 hunks)
- src/lzApp/OAppSenderUpgradeable.sol (5 hunks)
- test/foundry/unit/Bootstrap.t.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (2 hunks)
- test/foundry/unit/ExoCapsule.t.sol (4 hunks)
Additional context used
Learnings (2)
src/core/CustomProxyAdmin.sol (1)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#16 File: src/core/CustomProxyAdmin.sol:20-22 Timestamp: 2024-06-03T11:27:03.714Z Learning: The `CustomProxyAdmin` contract is designed for a `TransparentUpgradeableProxy` to upgrade itself. The `require(msg.sender == proxy)` condition in the `changeImplementation` function ensures that the proxy is the one initiating the upgrade, preventing unauthorized upgrades. This is a security measure to ensure that only the designated proxy can upgrade itself, and it can only do so once during its lifetime.src/core/ExocoreGateway.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#23 File: src/core/ExocoreGateway.sol:74-87 Timestamp: 2024-06-18T02:35:50.005Z Learning: User: adu-web3 PR: ExocoreNetwork/exocore-contracts#23 File: src/core/ExocoreGateway.sol:74-87 Timestamp: 2024-06-18T02:33:17.212Z Learning: An issue was created to track the optimization of the governance model by removing `exocoreValidatorSetAddress` and using a multi-sig wallet as the contract owner. This issue can be found at https://github.com/ExocoreNetwork/exocore-contracts/issues/30.
Additional comments not posted (19)
src/core/CustomProxyAdmin.sol (1)
35-35: Approved: Efficient use ofcalldatainchangeImplementation.The modification from
bytes memorytobytes calldatafor thedataparameter in thechangeImplementationfunction is a good practice for gas optimization in Ethereum contracts. The security checks ensuring that only the designated proxy can call this function remain intact, preserving the function's integrity.src/interfaces/IExoCapsule.sol (1)
48-48: Approved: Updated documentation forwithdraw.The clarification in the documentation of the
withdrawfunction to specify "unlocked staked ETH" enhances understanding and sets clear expectations for its usage.src/core/ClientChainGateway.sol (1)
118-118: Approved: Efficient use ofcalldatainquote.The modification from
bytes memorytobytes calldatafor the_messageparameter in thequotefunction is a good practice for gas optimization. This change preserves the function's logic and performance while reducing gas costs.src/lzApp/OAppSenderUpgradeable.sol (3)
22-22: Approved change for error declaration.Renaming
NotExactNativeFeetoIncorrectNativeFeebroadens the applicability of the error, enhancing clarity and usability.
Line range hint
73-86: Approved parameter renaming and logic adjustment in_lzSend.Renaming the parameter from
byApptopayByAppimproves clarity. The logic adjustment ensures correct fee handling based on whether the app or the app caller pays the fee. Verify that this change integrates well with other parts of the codebase.Also applies to: 90-90
Verification successful
Parameter Renaming Successfully Integrated
The renaming of the parameter from
byApptopayByAppin the_lzSendfunction has been successfully integrated into the codebase. The function calls reflect the updated parameter name, indicating correct propagation of the change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `_lzSend` function changes with related functions. # Test: Search for `_lzSend` usage in the codebase. Expect: Correct integration with other functions. rg --type solidity -A 5 $'_lzSend'Length of output: 66
Script:
#!/bin/bash # Find all Solidity files and search for the `_lzSend` function within them. fd --extension sol --exec rg '_lzSend' {}Length of output: 480
Line range hint
105-117: Approved logic adjustment in_payNative.The changes ensure that the function correctly handles fee payment scenarios, enhancing robustness. Verify that this change integrates well with other parts of the codebase, especially where
_payNativeis called.Verification successful
Integration of
_payNativeis correct and localized.The
_payNativefunction is used withinOAppSenderUpgradeable.solto handle message value calculations. Its integration appears consistent and should not affect other parts of the codebase. No further integration issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `_payNative` function changes with related functions. # Test: Search for `_payNative` usage in the codebase. Expect: Correct integration with other functions. rg --type solidity -A 5 $'_payNative'Length of output: 69
Script:
#!/bin/bash # Description: Verify integration of `_payNative` function changes with related functions. # Test: Search for `_payNative` usage in the codebase. Expect: Correct integration with other functions. rg '_payNative' -A 5Length of output: 948
src/libraries/Errors.sol (2)
26-28: Approved addition of errorNonZeroValue.The new error type enhances error handling by explicitly managing cases where a non-zero value is encountered.
28-28: Approved typographical correction in comment forZeroValue.The correction improves the clarity and professionalism of the documentation.
test/foundry/unit/ClientChainGateway.t.sol (1)
278-351: LGTM! Comprehensive testing of withdrawal functionality.The contract
withdrawNonBeaconChainETHFromCapsuleis well-structured and effectively tests both successful operations and expected failures of the withdrawal functionality. The use ofstdStoragefor direct storage manipulation is appropriate for testing scenarios.The code changes are approved.
Run the following script to verify the integration of this contract with other parts of the system:
Verification successful
Function
withdrawNonBeaconChainETHFromCapsuleis integrated and tested.The function
withdrawNonBeaconChainETHFromCapsuleis defined in the core logic and is used in the test file, indicating proper integration and testing within the system. The presence of access control modifiers further supports its intended use under specific conditions.
- Definition:
src/core/NativeRestakingController.sol- Usage:
test/foundry/unit/ClientChainGateway.t.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `withdrawNonBeaconChainETHFromCapsule` with other contracts. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type solidity -A 5 $'withdrawNonBeaconChainETHFromCapsule'Length of output: 95
Script:
#!/bin/bash # Description: Verify the integration of `withdrawNonBeaconChainETHFromCapsule` with other contracts. # Test: Search for the function usage without specifying the file type. rg -A 5 'withdrawNonBeaconChainETHFromCapsule'Length of output: 2497
src/core/ExoCapsule.sol (1)
271-278: LGTM! Improved clarity and security in function signature.The changes to
withdrawNonBeaconChainETHBalanceenhance clarity and enforce type safety by explicitly defining therecipientas apayableaddress. The update in the parameter name toamountToWithdrawalso clarifies the intent.The code changes are approved.
Run the following script to verify the integration of this function with other parts of the system:
test/foundry/unit/ExoCapsule.t.sol (1)
Line range hint
512-521: LGTM! Enhanced type safety in ETH withdrawal.The modifications to
withdrawNonBeaconChainETHBalanceensure that the recipient address is correctly cast to apayabletype, which is crucial for handling ETH transfers securely.The code changes are approved.
Run the following script to verify the integration of this function with other parts of the system:
Verification successful
Integration Verified:
withdrawNonBeaconChainETHBalanceFunctionThe
withdrawNonBeaconChainETHBalancefunction is well integrated across the codebase. It is defined, used, and tested appropriately, with error handling in place. No issues were found with its integration.
- Definition:
src/core/ExoCapsule.sol- Usage:
src/core/NativeRestakingController.sol- Testing:
test/foundry/unit/ExoCapsule.t.sol- Interface:
src/interfaces/IExoCapsule.sol- Error Handling:
src/libraries/Errors.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `withdrawNonBeaconChainETHBalance` with other contracts. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type solidity -A 5 $'withdrawNonBeaconChainETHBalance'Length of output: 91
Script:
#!/bin/bash # Description: Verify the integration of `withdrawNonBeaconChainETHBalance` with other contracts. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type sol -A 5 $'withdrawNonBeaconChainETHBalance'Length of output: 81
Script:
#!/bin/bash # Description: Verify the integration of `withdrawNonBeaconChainETHBalance` with other contracts. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg -A 5 $'withdrawNonBeaconChainETHBalance'Length of output: 3834
src/core/Bootstrap.sol (5)
329-331: Security Enhancement: Non-zero Ether CheckThe addition of a check for non-zero Ether values in the
depositfunction is a crucial security enhancement. It ensures that the function cannot be misused to send Ether unintentionally, aligning with best practices for contract security.
372-374: Consistent Security Practice: Non-zero Ether Check in Withdraw FunctionImplementing a check for non-zero Ether values in the
withdrawPrincipalFromExocorefunction is a consistent and necessary security measure. This prevents the function from processing Ether when it should not, maintaining the integrity of transaction handling.
438-440: Enhanced Contract Security: Non-zero Ether Check in Delegate FunctionThe inclusion of a non-zero Ether check in the
delegateTofunction is an excellent practice. It ensures that Ether is not mistakenly sent in a function that should not handle direct Ether transfers, enhancing the contract's security.
483-485: Security Consistency: Non-zero Ether Check in Undelegate FunctionAdding a check for non-zero Ether values in the
undelegateFromfunction is a vital security measure. It prevents unintended Ether transfers, ensuring that the function behaves as expected without handling Ether.
534-536: Comprehensive Security Update: Non-zero Ether Check in Combined Deposit/Delegate FunctionImplementing a non-zero Ether check in the
depositThenDelegateTofunction is a prudent security measure. It ensures that no Ether is mistakenly sent in a function designed only for token operations, maintaining the contract's integrity and security.test/foundry/unit/Bootstrap.t.sol (1)
1195-1240: Review of New Test Functions Handling Ether TransactionsThe newly added test functions correctly address the PR objectives by ensuring that operations involving Ether revert if a non-zero value is sent. This is crucial for maintaining the security and integrity of the contract, especially in scenarios where Ether should not be involved.
Each test function uses the
Errors.NonZeroValue.selectorto assert the expected reversion, which aligns with the best practices for error handling in Solidity. The use ofvm.startPrankandvm.stopPrankensures that state changes are isolated to the test environment, preventing side effects.Overall, the implementation of these test functions appears robust and well-structured. The use of explicit reversion checks for non-zero Ether values enhances the contract's resilience against unintended Ether transfers, which is a common security concern in smart contract development.
The code changes are approved as they effectively address the issues identified in the audit and improve the contract's security posture.
src/core/ExocoreGateway.sol (2)
603-605: Refund address handling improvement approved.The introduction of the
refundAddressvariable enhances clarity and security by explicitly determining the refund destination based on thepayByAppparameter. This change is aligned with the PR's objectives to enhance security and reliability.
610-610: Optimization of function parameter approved.Changing the parameter type from
bytes memorytobytes calldatain thequotefunction is a best practice for handling read-only data. This change optimizes gas usage and aligns with Solidity best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/core/ExoCapsule.sol (4 hunks)
- src/core/NativeRestakingController.sol (3 hunks)
- src/interfaces/IExoCapsule.sol (2 hunks)
- src/storage/ExoCapsuleStorage.sol (1 hunks)
- test/foundry/DepositWithdrawPrinciple.t.sol (1 hunks)
- test/foundry/unit/ExoCapsule.t.sol (6 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/core/ExoCapsule.sol
- src/core/NativeRestakingController.sol
- src/interfaces/IExoCapsule.sol
- test/foundry/unit/ExoCapsule.t.sol
Additional comments not posted (2)
src/storage/ExoCapsuleStorage.sol (1)
62-62: LGTM! But verify the usage ofcapsuleOwnerin the codebase.The change to make
capsuleOwnerapayable addressis appropriate for enabling Ether transfers. However, ensure that all references tocapsuleOwnerin the codebase correctly handle it as apayable address.The code changes are approved.
Run the following script to verify the usage of
capsuleOwner:Verification successful
The
capsuleOwneris correctly handled as apayable address.The references to
capsuleOwnerthroughout the codebase, including event emissions and Ether transfers, confirm that it is appropriately used as apayable address. The test files further validate this handling. No issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `capsuleOwner` handle it as a `payable address`. # Test: Search for the usage of `capsuleOwner`. Expect: Correct handling as a `payable address`. rg --type solidity -A 5 $'capsuleOwner'Length of output: 71
Script:
#!/bin/bash # Description: Verify all references to `capsuleOwner` handle it as a `payable address`. # Use ast-grep to search for the `capsuleOwner` variable and its usage in Solidity files. ast-grep --lang solidity --pattern $'$_ = capsuleOwner'Length of output: 173
Script:
#!/bin/bash # Description: Search for the usage of `capsuleOwner` in Solidity files. # Use grep to search for the term `capsuleOwner` in all Solidity files. grep -r 'capsuleOwner' --include \*.solLength of output: 2778
test/foundry/DepositWithdrawPrinciple.t.sol (1)
464-464: LGTM! But verify the usage ofinitializein the codebase.The change to make the
depositorAddrargument apayable addressis appropriate for enabling Ether transfers. However, ensure that all references toinitializein the codebase correctly handle thepayable addressargument.The code changes are approved.
Run the following script to verify the usage of
initialize:Verification successful
The usage of
initializewithpayableaddresses is verified.The search results confirm that the
initializefunction is used withpayableaddresses in several instances, including the one involvingcapsule.initialize. This indicates that the change is intentional and correctly implemented for handling Ether transfers. No issues were found regarding the handling ofpayableaddresses in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `initialize` handle the `depositorAddr` argument as a `payable address`. # Test: Search for the usage of `initialize`. Expect: Correct handling with `payable address` argument. rg --type solidity -A 5 $'initialize'Length of output: 69
Script:
#!/bin/bash # Find all Solidity files and search for the usage of the `initialize` function. fd -e sol -x rg 'initialize' {}Length of output: 5647
Description
There are some minor issues identified in the audit by certik, we could address them in one PR as they are not complex or hard to fix
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests