fix: native restaking withdrawal checks could be bypassed #85
Conversation
…d to bypass native restaking withdrawal checks
WalkthroughThe changes introduce enhanced security measures in the Changes
Assessment against linked issues
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 (
|
adu-web3
requested review from
MaxMustermann2,
magj2006,
bwhour and
cloud8little
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
src/core/LSTRestakingController.sol (1)
51-54: Security enhancement in withdrawal function.The addition of
_getVault(token);inwithdrawPrincipalFromExocorefunction is a crucial security enhancement. It ensures that only tokens associated with a valid vault can be used, which indirectly prevents the use ofVIRTUAL_STAKED_ETH_ADDRESSfor unauthorized withdrawals.The code changes are approved.
Consider adding explicit error handling if the vault is not found, to provide clearer feedback to the user:
IVault vault = _getVault(token); require(address(vault) != address(0), "Invalid token: Vault not found");test/foundry/unit/ClientChainGateway.t.sol (2)
Line range hint
280-354: Enhanced clarity in contract naming and comprehensive test coverage.The renaming of the contract to
WithdrawNonBeaconChainETHFromCapsulefollows Solidity's convention and enhances clarity. The test scenarios cover successful withdrawals and various failure modes effectively.The code changes are approved.
Consider adding more detailed comments for each test scenario to provide better context and understanding of the expected behaviors and conditions:
// Test to ensure successful withdrawal of non-beacon chain ETH from a capsule. function test_success_withdrawNonBeaconChainETH() public { ... } // Test to ensure the function reverts if the capsule is not found. function test_revert_capsuleNotFound() public { ... }
355-408: Robust testing for new security measures in withdrawal scenarios.The
WithdrawalPrincipalFromExocorecontract provides comprehensive testing for the new security measures. It ensures that the withdrawal logic is robust and handles edge cases appropriately, such as whitelisting checks and zero amount withdrawals.The code changes are approved.
Enhance the clarity of test descriptions to better convey the purpose and expected outcomes of each test:
// Test to ensure the function reverts when attempting to withdraw a non-whitelisted token. function test_revert_withdrawNonWhitelistedToken() public { ... } // Test to ensure the function reverts when attempting to withdraw zero amount. function test_revert_withdrawZeroAmount() public { ... }
|
in this new commit, we forbid deploying vault for We should forbid deploying vault for |
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/Bootstrap.sol (1 hunks)
- src/core/ClientGatewayLzReceiver.sol (1 hunks)
- src/libraries/Errors.sol (1 hunks)
- src/storage/BootstrapStorage.sol (3 hunks)
- src/storage/ClientChainGatewayStorage.sol (1 hunks)
- test/foundry/unit/Bootstrap.t.sol (2 hunks)
Files skipped from review due to trivial changes (2)
- src/core/ClientGatewayLzReceiver.sol
- src/storage/ClientChainGatewayStorage.sol
Additional comments not posted (7)
src/libraries/Errors.sol (1)
210-211: LGTM!The new error declaration
ForbidToDeployVaultis a valuable addition to theErrorslibrary. It follows the naming convention, is placed in the appropriate location, and has a clear error message. This error will enhance the error handling capabilities of the library and improve the clarity of the code when dealing with vault deployments related to staked ETH.src/storage/BootstrapStorage.sol (3)
8-9: LGTM!The import statement for the
Errorslibrary is necessary and looks good.
151-152: LGTM!The constant
VIRTUAL_STAKED_ETH_ADDRESSis defined correctly and serves a clear purpose in the contract.
343-345: LGTM!The if statement correctly prevents deploying vaults with the
VIRTUAL_STAKED_ETH_ADDRESStoken, enhancing the contract's security.src/core/Bootstrap.sol (1)
198-200: LGTM!The added conditional check is necessary to prevent deploying a vault for the virtual token address representing natively staked ETH. It ensures that the vault deployment logic is skipped for this specific token, while allowing it for other tokens that don't have a vault yet.
test/foundry/unit/Bootstrap.t.sol (2)
68-68: LGTM!The constant
VIRTUAL_STAKED_ETH_ADDRESSis correctly defined and will be useful for handling the specific case related to the virtual token in the test suite.
180-192: LGTM!The test
test02_VaultNotDeployedForNativeStakedETHis correctly implemented and verifies that a vault is not deployed for the virtual staked ETH address. This improves the robustness of the contract by ensuring that the system correctly identifies and handles the virtual staked ETH scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/mocks/NonShortCircuitEndpointV2Mock.sol (1 hunks)
Files skipped from review due to trivial changes (1)
- test/mocks/NonShortCircuitEndpointV2Mock.sol
Description
closes: #84
the solution is simple, before sending the withdrawal request to Exocore in
LSTRestakingController.withdrawPrincipalFromExocore, we try to get corresponding vault for the specified token. If we could get the vault, the token cannot be VIRTUAL_STAKED_ETH_ADDRESS.Simply checking that the token being not equal to VIRTUAL_STAKED_ETH_ADDRESS is also a good approach, but
_getVaultcould guarantee not only the token is LST token but also the vault existsSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation