Integrate WithdrawablePeriphery [ERC20Proxy v1.1.0,Executor v2.1.0,LiFiDEXAggregator v1.6.0,Receiver v2.1.0,ReceiverAcrossV3 v1.1.0,ReceiverStargateV2 v1.1.0,RelayerCelerIM v2.1.1,TokenWrapper v1.1.0] #909
Conversation
WalkthroughThis pull request introduces significant refactoring across multiple peripheral contracts in the LiFi protocol. The primary change involves replacing existing ownership management with a new Changes
Possibly related PRs
Suggested labels
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 (
|
lifi-action-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/Periphery/ReceiverAcrossV3.sol (1)
36-36: Validate _owner to ensure non-zero address
Consider enforcing a require statement or similar guard for _owner in the constructor to prevent accidental zero-address ownership.src/Periphery/ReceiverStargateV2.sol (1)
66-66: _owner constructor parameter
Similar to ReceiverAcrossV3, consider adding a defense check to prevent setting _owner as address(0).src/Periphery/Receiver.sol (1)
53-53: _owner constructor check
As with other additions, consider validating _owner to avoid accidental zero-owner scenarios.src/Periphery/RelayerCelerIM.sol (1)
47-47: Initializing the _owner address
Include additional checks to avoid inadvertently setting an empty owner.src/Periphery/LiFiDEXAggregator.sol (1)
25-26: Version bump alignment
The version increment from 1.5.0 to 1.6.0 is consistent with the shift to WithdrawablePeriphery. Confirm that this major change is adequately reflected in documentation and release notes.src/Periphery/ERC20Proxy.sol (2)
5-5: Inheritance import
The introduction of WithdrawablePeriphery sets a new ownership model for ERC20Proxy. Ensure that existing calls or references to the old ownership checks are fully removed, such as the old Ownable references.
10-11: Version bump and ownership
Updating from 1.0.0 to 1.1.0 is consistent with the shift to WithdrawablePeriphery. Double-check that the contract’s external documentation and release notes mention this ownership modification.test/solidity/Periphery/TokenWrapper.t.sol (1)
18-18: TokenWrapper instantiation
Initializing the TokenWrapper with this contract as _owner ensures that the test harness can manage ownership. Confirm that any production usage initializes a real (non-test) owner address.src/Periphery/TokenWrapper.sol (1)
18-19: Version bump
Increasing from 1.0.0 to 1.1.0 aligns with the new withdrawal management. Ensure the upgrade notice is included in docs or release notes for clarity on backward compatibility or migration steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Periphery/ERC20Proxy.sol(1 hunks)src/Periphery/Executor.sol(2 hunks)src/Periphery/LiFiDEXAggregator.sol(3 hunks)src/Periphery/Receiver.sol(2 hunks)src/Periphery/ReceiverAcrossV3.sol(2 hunks)src/Periphery/ReceiverStargateV2.sol(3 hunks)src/Periphery/RelayerCelerIM.sol(4 hunks)src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Facets/CelerIMFacet.t.sol(1 hunks)test/solidity/Periphery/Executor.t.sol(1 hunks)test/solidity/Periphery/Receiver.t.sol(3 hunks)test/solidity/Periphery/ReceiverAcrossV3.t.sol(5 hunks)test/solidity/Periphery/ReceiverStargateV2.t.sol(5 hunks)test/solidity/Periphery/RelayerCelerIM.t.sol(1 hunks)test/solidity/Periphery/TokenWrapper.t.sol(3 hunks)
🔇 Additional comments (32)
src/Periphery/ReceiverAcrossV3.sol (2)
8-8: Confirm that the new inheritance is fully compatible
With this new import of WithdrawablePeriphery, please confirm that the ownership checks and withdrawal patterns remain consistent with the contract’s intended logic.
15-16: Version bump looks good
No issues spotted with incrementing the version to 1.1.0.
src/Periphery/ReceiverStargateV2.sol (3)
10-10: New inheritance import
Ensure that references to the previous ownership scheme have been fully removed and that the WithdrawablePeriphery functionality is adequately tested.
37-37: Version updated to 1.1.0
No concerns regarding the version change.
40-40: Replaced TransferrableOwnership with WithdrawablePeriphery
Confirm that all ownership checks and modifiers are updated to rely on the new parent contract’s security model.
src/Periphery/Receiver.sol (3)
10-10: Importing WithdrawablePeriphery
Please ensure that any leftover references to TransferrableOwnership or old ownership logic have been removed or converted.
16-16: Version increment to 2.1.0
No specific remarks; versioning seems appropriate.
17-17: Extends WithdrawablePeriphery
The new ownership approach appears consistent with the rest of the PR; verify that unit tests confirm the updated owner-based access controls.
src/Periphery/RelayerCelerIM.sol (4)
12-12: Adoption of WithdrawablePeriphery
Confirm that no references to TransferrableOwnership or its functions remain, and that the new withdrawal flow is fully tested.
21-21: Bumped contract version
No issues with the minor version increment to 2.1.0.
22-22: Transition to new inheritance
Ensure that existing “onlyOwner” calls properly map to WithdrawablePeriphery’s ownership model.
431-435: Fallback address usage
Using msg.sender when _to is zero helps prevent funds from being stuck. This is a good safety measure.
src/Periphery/LiFiDEXAggregator.sol (3)
6-6: Replace direct reference to WithdrawablePeriphery
This import confirms that the contract now extends the associated withdrawal and ownership logic. Ensure that the new inheritance does not introduce any vulnerabilities or override critical functionalities from other base contracts.
Would you like a script to scan for references to the old ownership implementation to ensure full migration?
60-60: Privileged user check
Switching from owner() to owner introduces an inline variable reference. Confirm that “owner” is properly set in the WithdrawablePeriphery constructor. This approach simplifies the call but ensure you keep consistent usage (no accidental fallback to old Ownable patterns).
Please confirm by reviewing all references to 'owner' in the codebase to ensure that they align with the new inheritance.
66-70: Adjust constructor for new inheritance
This adjusted constructor ensures the _owner parameter initializes the WithdrawablePeriphery. Make sure that external deployment scripts and tests pass the correct owner address (e.g., a multisig) and not unintentionally the deployer’s EOA.
src/Periphery/ERC20Proxy.sol (1)
19-19: Constructor alignment
This new constructor passes _owner to WithdrawablePeriphery. Verify that any references to old ownership variables or functionalities are either removed or updated, eliminating confusion between old vs. new ownership flows.
test/solidity/Periphery/TokenWrapper.t.sol (2)
9-9: Import usage
The import of IERC20 and LibAsset indicates you might be targeting direct asset transfers within your tests. Confirm you’re leveraging these for the new withdrawal logic or token validations if needed.
31-44: New withdrawal test
This new test case, “testCanWithdrawToken,” is a beneficial addition for coverage. It validates that ETH can enter the TokenWrapper and be withdrawn properly. Ensure you add similar tests for ERC20 tokens if your contract is intended to handle them as well.
src/Periphery/Executor.sol (2)
10-10: LGTM! Proper integration of WithdrawablePeriphery
The contract correctly inherits from WithdrawablePeriphery and includes the necessary import. The version bump to 2.1.0 appropriately reflects the addition of new functionality without breaking changes.
Also applies to: 19-25
74-77: Verify owner initialization across the codebase
The constructor has been updated to include the owner parameter, which is correctly passed to WithdrawablePeriphery.
✅ Verification successful
Owner initialization is consistently implemented across the codebase
All contracts inheriting from WithdrawablePeriphery correctly pass the owner parameter to the parent constructor. This includes:
- ERC20Proxy
- Executor
- GasZipPeriphery
- LiFiDEXAggregator
- Permit2Proxy
- ReceiverAcrossV3
- RelayerCelerIM
- TokenWrapper
The implementation in Executor.sol follows the same pattern as all other contracts in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent owner initialization across the codebase
# Look for other contracts inheriting from WithdrawablePeriphery and check their constructors
ast-grep --pattern 'contract $_ is $_WithdrawablePeriphery$_'
Length of output: 63
Script:
#!/bin/bash
# Let's try a different approach to find contracts inheriting from WithdrawablePeriphery
# First, let's find the WithdrawablePeriphery contract
echo "=== Finding WithdrawablePeriphery contract ==="
fd "WithdrawablePeriphery.sol"
# Then search for contract inheritance patterns
echo -e "\n=== Finding contracts inheriting from WithdrawablePeriphery ==="
rg "contract.*WithdrawablePeriphery" -A 5
# Also search for constructor patterns in files that mention WithdrawablePeriphery
echo -e "\n=== Finding constructors in files mentioning WithdrawablePeriphery ==="
rg "WithdrawablePeriphery" --files-with-matches | xargs rg "constructor.*\(" -A 5
Length of output: 9293
test/solidity/Periphery/ReceiverAcrossV3.t.sol (3)
33-33: LGTM! Proper constructor initialization
The Executor constructor is correctly initialized with both the ERC20Proxy address and owner parameter.
64-64: LGTM! Consistent method renaming
Test methods and calls have been properly updated from pullToken to withdrawToken to match the new interface.
Also applies to: 73-73, 78-78, 81-81
Line range hint 193-194: Debug Transfer event test
There's a commented test case with a note requesting debug help. This should be investigated.
Would you like help debugging the Transfer event test case?
test/solidity/Periphery/Receiver.t.sol (2)
47-47: LGTM! Proper constructor initialization
The Executor constructor is correctly initialized with both the ERC20Proxy address and owner parameter.
64-64: LGTM! Consistent method renaming
Test methods and calls have been properly updated from pullToken to withdrawToken to match the new interface.
Also applies to: 73-73, 78-78, 81-81
test/solidity/Periphery/RelayerCelerIM.t.sol (1)
47-47: LGTM: Constructor update is consistent.
The Executor constructor now includes the owner parameter, maintaining consistency with the WithdrawablePeriphery integration.
test/solidity/Periphery/ReceiverStargateV2.t.sol (2)
47-47: LGTM: Constructor update is consistent.
The Executor constructor now includes the owner parameter, maintaining consistency with the WithdrawablePeriphery integration.
Line range hint 62-110: LGTM: Method names updated to match WithdrawablePeriphery.
Test method names have been consistently updated from pullToken to withdrawToken to align with the WithdrawablePeriphery contract integration. The test coverage remains comprehensive, including:
- ERC20 token withdrawal
- Native token withdrawal
- Failed external call handling
- Authorization checks
test/solidity/Facets/CelerIMFacet.t.sol (2)
104-104: LGTM: Constructor update is consistent.
The Executor constructor now includes the owner parameter, maintaining consistency with the WithdrawablePeriphery integration.
Line range hint 39-56: Verify the event handling in the contract implementation.
The test defines two Burn events with different parameter lists:
- Local burn event with withdrawal account
- Cross-chain burn event with chain ID and nonce
While this is valid Solidity, ensure that the contract implementation correctly emits the appropriate event variant based on the burn type.
test/solidity/Periphery/Executor.t.sol (2)
73-73: LGTM! Constructor change maintains existing functionality.
The addition of the owner parameter aligns with the WithdrawablePeriphery integration while preserving the existing test scenarios.
Line range hint 1-600: Verify external call validation in test scenarios.
The test suite thoroughly covers various swap scenarios but should explicitly verify that arbitrary external calls are properly validated and restricted, as emphasized in the PR objectives.
Let's analyze the external calls in the test suite:
✅ Verification successful
Let me gather more information about the validation mechanisms in the codebase.
Based on the code review, I can now provide the final response:
External call validation is properly implemented in the Executor contract
The test scenarios in Executor.t.sol are well-aligned with the security measures implemented in the contract:
- The contract includes a specific validation check that prevents direct calls to the ERC20Proxy (
if (_swapData[i].callTo == address(erc20Proxy)) { revert UnAuthorized(); }) - The test
testFailWhenCallingERC20ProxyDirectly()explicitly verifies this validation - All external calls are executed through the
LibSwap.swap()function with proper validation context - The contract includes the
ReentrancyGuardmodifier on public functions - The
noLeftoversmodifier ensures proper balance management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for external calls in swap data to ensure they're properly validated
rg -A 2 "SwapData\(" test/solidity/Periphery/Executor.t.sol
# Search for validation checks in the Executor contract
ast-grep --pattern 'function validateCall($_) {
$$$
}'
Length of output: 2156
Script:
#!/bin/bash
# Search for the Executor contract implementation to understand validation
fd -e sol Executor.sol
# Check the contract's validation mechanisms
rg -A 10 "function swapAndCompleteBridgeTokens|function swapAndExecute" --type sol
Length of output: 234
Script:
#!/bin/bash
# Check the Executor contract implementation
cat src/Periphery/Executor.sol
# Look for validation patterns in the interface
cat src/Interfaces/IExecutor.sol
# Search for validation patterns with proper file type
rg -t sol -A 10 "function swapAndCompleteBridgeTokens|function swapAndExecute"
Length of output: 10086
Test Coverage ReportLine Coverage: 78.74% (2200 / 2794 lines) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
script/deploy/zksync/DeployTokenWrapper.s.sol (1)
51-51: Add documentation for the return value structure.The return value now includes multiple parameters. Consider adding a comment explaining the structure of the encoded constructor arguments for better maintainability.
+// Encode constructor arguments: (address wrappedNativeAddress, address refundWalletAddress) return abi.encode(wrappedNativeAddress, refundWalletAddress);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/facets/DeployTokenWrapper.s.sol(2 hunks)script/deploy/zksync/DeployTokenWrapper.s.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (6)
script/deploy/facets/DeployTokenWrapper.s.sol (3)
Line range hint
1-53: Overall changes look good!The modifications properly support the integration of WithdrawablePeriphery by adding the necessary configuration handling for the refund wallet address. The code maintains consistent patterns and is well-structured.
51-51: Verify constructor parameter order.Please ensure that the order of encoded parameters (
wrappedNativeAddress, refundWalletAddress) matches the TokenWrapper contract's constructor parameter order.✅ Verification successful
Parameter order is correct ✅
The order of parameters in
abi.encode(wrappedNativeAddress, refundWalletAddress)perfectly matches theTokenWrapperconstructor's parameter order:constructor(address _wrappedToken, address _owner).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check TokenWrapper constructor signature ast-grep --pattern 'contract TokenWrapper { constructor($params) { $$$ } $$$ }'Length of output: 91
Script:
#!/bin/bash # Find TokenWrapper contract file fd -e sol TokenWrapper # Once found, let's examine its content fd -e sol TokenWrapper -x cat {}Length of output: 7618
29-33: Verify the existence of global.json configuration file.The script assumes the existence of a global configuration file at
/config/global.json. Let's verify this assumption.✅ Verification successful
Global configuration file verified successfully
The
global.jsonconfiguration file exists at the expected location and contains the necessary configuration data. The path construction pattern is consistent with other deployment scripts in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the global config file exists fd -t f "global.json" -x echo "Found global config at: {}"Length of output: 103
Script:
#!/bin/bash # Check the content of global.json and look for other references cat ./config/global.json echo "---" rg "global.json" --type-add 'sol:*.sol' -t solLength of output: 10054
script/deploy/zksync/DeployTokenWrapper.s.sol (3)
23-33: LGTM! Improved configuration file organization.Good separation of concerns by splitting configuration into separate files. The variable rename from
networkConfigtotokenWrapperConfigbetter reflects its specific purpose.
36-41: LGTM! Safe configuration reading.The code safely reads and extracts the wrapped native token address using type-safe methods.
Line range hint
1-53: Verify deployment script updates across environments.Since this is part of a larger refactoring where TokenWrapper now inherits from WithdrawablePeriphery, ensure that:
- All environment-specific deployment scripts are updated
- Test fixtures are aligned with the new constructor parameters
✅ Verification successful
All deployment scripts and tests are properly aligned
The deployment scripts in both environments (zksync and facets) use identical and correct constructor parameters, and the test file correctly instantiates TokenWrapper with the expected parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other deployment scripts that might need updates fd -e sol -e js "Deploy.*TokenWrapper" --exec rg -l "TokenWrapper" # Check test files for constructor usage rg -l "new TokenWrapper" "test/"Length of output: 238
Script:
#!/bin/bash # Check the other deployment script cat script/deploy/facets/DeployTokenWrapper.s.sol # Check constructor usage in test ast-grep --pattern 'new TokenWrapper($$$)' test/solidity/Periphery/TokenWrapper.t.solLength of output: 1832
Script:
#!/bin/bash # View test file content cat test/solidity/Periphery/TokenWrapper.t.solLength of output: 1953
|
In |
…grate-withdrawable-periphery
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
audit/reports/2025.01.09_WithdrawablePeripheryUpdates.pdfis excluded by!**/*.pdf
📒 Files selected for processing (1)
audit/auditLog.json(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (1)
audit/auditLog.json (1)
Line range hint
125-208: LGTM! Contract versions and audit associations are properly tracked.The audit log correctly tracks version changes and audit associations for all contracts that have been updated to use WithdrawablePeriphery:
- ERC20Proxy v1.1.0
- Executor v2.1.0
- LiFiDEXAggregator v1.6.0
- ReceiverAcrossV3 v1.1.0
- ReceiverStargateV2 v1.1.0
- Receiver v2.1.0
- RelayerCelerIM v2.1.0
- TokenWrapper v1.1.0
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Periphery/LiFiDEXAggregator.sol (1)
62-62: Fix typo in "privileged" throughout the contract.The word "privileged" is consistently misspelled as "priviledged" in variable names, function names, and error messages.
Apply this diff to fix the typo:
- msg.sender == owner || priviledgedUsers[msg.sender], + msg.sender == owner || privilegedUsers[msg.sender], - address[] memory priviledgedUserList, + address[] memory privilegedUserList, - priviledgedUsers[priviledgedUserList[i]] = true; + privilegedUsers[privilegedUserList[i]] = true;Also applies to: 70-70, 81-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
audit/auditLog.json(6 hunks)src/Periphery/LiFiDEXAggregator.sol(3 hunks)src/Periphery/Receiver.sol(2 hunks)src/Periphery/ReceiverAcrossV3.sol(2 hunks)src/Periphery/ReceiverStargateV2.sol(3 hunks)src/Periphery/RelayerCelerIM.sol(4 hunks)src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Periphery/ReceiverAcrossV3.t.sol(5 hunks)test/solidity/Periphery/ReceiverStargateV2.t.sol(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Periphery/TokenWrapper.sol
- test/solidity/Periphery/ReceiverStargateV2.t.sol
- audit/auditLog.json
🧰 Additional context used
📓 Learnings (1)
src/Periphery/LiFiDEXAggregator.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:15-21
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In Solidity, events cannot be imported from another contract; they need to be redefined or imported from an interface.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
🪛 GitHub Actions: Enforce Min Test Coverage
test/solidity/Periphery/ReceiverAcrossV3.t.sol
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (25)
src/Periphery/ReceiverAcrossV3.sol (4)
8-8: LGTM!The import statement is correctly added for the new inheritance.
15-15: LGTM!Version bump from 1.0.3 to 1.1.0 is appropriate for this feature change.
16-16: LGTM!The inheritance change aligns with the PR objectives to integrate WithdrawablePeriphery.
36-36: LGTM!The constructor initialization is correctly updated to match the new inheritance.
test/solidity/Periphery/ReceiverAcrossV3.t.sol (5)
33-33: LGTM!The Executor constructor call is correctly updated with the required owner parameter.
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
78-78: LGTM!The method name is correctly updated to match the WithdrawablePeriphery interface.
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
93-96: LGTM!The method name is correctly updated to match the WithdrawablePeriphery interface.
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
103-103: LGTM!The method name is correctly updated to match the WithdrawablePeriphery interface.
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
64-64:⚠️ Potential issueFix the test failure.
The test is failing because the error message in
test_PullTokenWillRevertIfExternalCallFailsexpectsETHTransferFailed()but receivesExternalCallFailed(). This indicates that the error handling in WithdrawablePeriphery differs from the original implementation.Update the test to expect the correct error:
- vm.expectRevert(abi.encodeWithSignature("ETHTransferFailed()")); + vm.expectRevert(ExternalCallFailed.selector);Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
src/Periphery/ReceiverStargateV2.sol (4)
11-11: LGTM!The import statement is correctly added for the new inheritance.
38-38: LGTM!Version bump from 1.0.1 to 1.1.0 is appropriate for this feature change.
41-41: LGTM!The inheritance change aligns with the PR objectives to integrate WithdrawablePeriphery.
67-67: LGTM!The constructor initialization is correctly updated to match the new inheritance.
src/Periphery/Receiver.sol (4)
11-11: LGTM!The import statement is correctly added for the new inheritance.
17-17: LGTM!Version bump from 2.0.3 to 2.1.0 is appropriate for this feature change.
18-18: LGTM!The inheritance change aligns with the PR objectives to integrate WithdrawablePeriphery.
54-54: LGTM!The constructor initialization is correctly updated to match the new inheritance.
src/Periphery/RelayerCelerIM.sol (5)
13-13: LGTM!The import statement is correctly added for the new inheritance.
22-22: LGTM!Version bump from 2.0.1 to 2.1.1 is appropriate for this feature change.
23-23: LGTM!The inheritance change aligns with the PR objectives to integrate WithdrawablePeriphery.
48-48: LGTM!The constructor initialization is correctly updated to match the new inheritance.
428-432: LGTM!The event emission is correctly updated to use the standardized TokensWithdrawn event.
src/Periphery/LiFiDEXAggregator.sol (3)
27-27: LGTM! Version bump is appropriate.The version bump from 1.5.1 to 1.6.0 correctly reflects the significant changes in the contract's inheritance structure and functionality.
68-72: LGTM! Constructor changes are well-implemented.The constructor correctly initializes the
WithdrawablePeripherybase contract with the provided owner address.
62-62: LGTM! Modifier changes are correct.The modifier correctly uses the
ownerstate variable fromWithdrawablePeripheryinstead of theowner()function fromOwnable.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
Line range hint
89-97: Fix failing test: Update expected error messageThe test is failing because it expects
ETHTransferFailed()but receivesExternalCallFailed(). This change in error message likely comes from the WithdrawablePeriphery integration. Update the expected error:-vm.expectRevert(abi.encodeWithSignature("ETHTransferFailed()")); +vm.expectRevert(abi.encodeWithSignature("ExternalCallFailed()"));
🧹 Nitpick comments (2)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
64-64: Update test function names to match new terminologyWhile the function calls have been updated from
pullTokentowithdrawToken, the test function names still use the old terminology (e.g.,test_OwnerCanPullERC20Token). Consider updating these for consistency:
test_OwnerCanPullERC20Token→test_OwnerCanWithdrawERC20Tokentest_OwnerCanPullNativeToken→test_OwnerCanWithdrawNativeTokentest_PullTokenWillRevertIfExternalCallFails→test_WithdrawTokenWillRevertIfExternalCallFailstest_revert_PullTokenNonOwner→test_revert_WithdrawTokenNonOwnerAlso applies to: 78-78, 93-93, 103-103
🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 64-64: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
src/Periphery/LiFiDEXAggregator.sol (1)
Line range hint
62-81: Fix typo in variable names and function names.The word "priviledged" is misspelled throughout the contract. It should be "privileged". This affects:
- Variable name
priviledgedUsers- Function name
setPriviledge- Parameter name
priviledgedUserList- Error message text
Apply this diff to fix the typos:
- mapping(address => bool) public priviledgedUsers; + mapping(address => bool) public privilegedUsers; - modifier onlyOwnerOrPriviledgedUser() { + modifier onlyOwnerOrPrivilegedUser() { require( msg.sender == owner || priviledgedUsers[msg.sender], - "RP: caller is not the owner or a privileged user" + "RP: caller is not the owner or a privileged user" ); _; } constructor( address _bentoBox, - address[] memory priviledgedUserList, + address[] memory privilegedUserList, address _owner ) WithdrawablePeriphery(_owner) { bentoBox = IBentoBoxMinimal(_bentoBox); lastCalledPool = IMPOSSIBLE_POOL_ADDRESS; - for (uint256 i = 0; i < priviledgedUserList.length; i++) { - priviledgedUsers[priviledgedUserList[i]] = true; + for (uint256 i = 0; i < privilegedUserList.length; i++) { + privilegedUsers[privilegedUserList[i]] = true; } } - function setPriviledge(address user, bool priviledge) external onlyOwner { - priviledgedUsers[user] = priviledge; + function setPrivilege(address user, bool privilege) external onlyOwner { + privilegedUsers[user] = privilege; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
audit/auditLog.json(6 hunks)src/Periphery/LiFiDEXAggregator.sol(3 hunks)src/Periphery/Receiver.sol(2 hunks)src/Periphery/ReceiverAcrossV3.sol(2 hunks)src/Periphery/ReceiverStargateV2.sol(3 hunks)src/Periphery/RelayerCelerIM.sol(4 hunks)src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Periphery/ReceiverAcrossV3.t.sol(5 hunks)test/solidity/Periphery/ReceiverStargateV2.t.sol(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/solidity/Periphery/ReceiverStargateV2.t.sol
- audit/auditLog.json
🧰 Additional context used
📓 Learnings (2)
src/Periphery/TokenWrapper.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
src/Periphery/LiFiDEXAggregator.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:15-21
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In Solidity, events cannot be imported from another contract; they need to be redefined or imported from an interface.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
🪛 GitHub Actions: SC Core Dev Approval Check
src/Periphery/LiFiDEXAggregator.sol
[warning] 62-81: Typo in variable names and function names: 'priviledged' should be 'privileged' throughout the contract
test/solidity/Periphery/ReceiverAcrossV3.t.sol
[error] 64-64: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
🪛 GitHub Actions: Enforce Min Test Coverage
test/solidity/Periphery/ReceiverAcrossV3.t.sol
[error] 1-1: Test failure: test_PullTokenWillRevertIfExternalCallFails() - Expected error ETHTransferFailed() but got ExternalCallFailed()
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (14)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
33-33: LGTM: Constructor update aligns with WithdrawablePeriphery integrationThe additional parameter in the Executor constructor properly initializes the contract with the test contract address.
src/Periphery/RelayerCelerIM.sol (1)
23-23: Verify the impact of changing inheritance toWithdrawablePeriphery.Switching the base contract to
WithdrawablePeripherymodifies the inheritance hierarchy and may affect access control and withdrawal logic. Please ensure that all inherited functions and modifiers behave as intended, and that this change does not introduce any unintended side effects.src/Periphery/TokenWrapper.sol (2)
6-6: Integration withWithdrawablePeripheryis appropriate.The import and inheritance from
WithdrawablePeripherycorrectly incorporate withdrawal functionality into theTokenWrappercontract.
29-32: Constructor updated with_ownerparameter is correct.Adding the
_ownerparameter and passing it toWithdrawablePeripheryaligns with the new inheritance structure and ensures proper ownership initialization.src/Periphery/ReceiverAcrossV3.sol (4)
8-8: Inheritance fromWithdrawablePeripheryis properly implemented.Updating the import and inheritance reflects the new structure for ownership and withdrawal management.
16-16: Version number updated appropriately.The version annotation accurately reflects the changes introduced in this update.
36-36: Constructor correctly initializesWithdrawablePeriphery.Passing
_ownerto theWithdrawablePeripheryconstructor ensures proper setup of ownership.
Line range hint
58-66: Remove redundantpullTokenfunction due to inherited withdrawal functionality.With the contract now inheriting from
WithdrawablePeriphery, thepullTokenfunction duplicates the withdrawal capabilities provided by the base contract. Removing this function will prevent redundancy and possible conflicts.src/Periphery/ReceiverStargateV2.sol (2)
11-11: LGTM! Inheritance and version changes look good.The contract now inherits from
WithdrawablePeripheryand the version has been appropriately bumped to 1.1.0 to reflect the new functionality.Also applies to: 38-38, 41-41
67-67: LGTM! Constructor properly initializes WithdrawablePeriphery.The constructor correctly passes the owner parameter to the
WithdrawablePeripherybase contract.src/Periphery/Receiver.sol (2)
11-11: LGTM! Inheritance and version changes look good.The contract now inherits from
WithdrawablePeripheryand the version has been appropriately bumped to 2.1.0 to reflect the new functionality.Also applies to: 17-18
54-54: LGTM! Constructor properly initializes WithdrawablePeriphery.The constructor correctly passes the owner parameter to the
WithdrawablePeripherybase contract.src/Periphery/LiFiDEXAggregator.sol (2)
6-6: LGTM! Inheritance and version changes look good.The contract now inherits from
WithdrawablePeripheryand the version has been appropriately bumped to 1.6.0 to reflect the new functionality.Also applies to: 27-28
68-72: LGTM! Constructor properly initializes WithdrawablePeriphery.The constructor correctly accepts and passes the owner parameter to the
WithdrawablePeripherybase contract.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
Line range hint
64-86: Add test cases for WithdrawablePeriphery functionality.Consider adding the following test cases:
- Test withdrawing tokens with amount = 0
- Test withdrawing non-existent tokens
- Test withdrawing more tokens than available
- Test withdrawing to address(0)
Example test case:
function test_revert_WithdrawTokenZeroAmount() public { vm.startPrank(USER_DIAMOND_OWNER); vm.expectRevert(InvalidAmount.selector); receiver.withdrawToken(ADDRESS_DAI, payable(USER_RECEIVER), 0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Periphery/ReceiverAcrossV3.sol(3 hunks)test/solidity/Periphery/ReceiverAcrossV3.t.sol(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
- GitHub Check: analyze
🔇 Additional comments (5)
src/Periphery/ReceiverAcrossV3.sol (4)
15-15: LGTM: Version bump follows semantic versioning.The update from 1.0.3 to 1.1.0 appropriately reflects the addition of WithdrawablePeriphery functionality.
36-36: LGTM: Constructor properly initializes WithdrawablePeriphery.The redundant owner initialization has been removed as discussed in previous reviews, and the parent constructor is correctly used.
76-76: LGTM: Clear documentation about native token handling.The added comment clarifies the contract's behavior with wrapped native tokens, which is valuable for maintainers.
8-8: LGTM: Ownership management refactored to use WithdrawablePeriphery.The change from
TransferrableOwnershiptoWithdrawablePeripheryaligns with the architectural changes across the codebase.Let's verify that all contracts using
TransferrableOwnershiphave been migrated:Also applies to: 16-16
✅ Verification successful
Architectural enhancement: WithdrawablePeriphery extends TransferrableOwnership
The change is correct as WithdrawablePeriphery is an extension of TransferrableOwnership, adding withdrawal capabilities to specific contracts that need it, while other contracts continue to use TransferrableOwnership directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining TransferrableOwnership usage rg "TransferrableOwnership" --type solLength of output: 66
Script:
#!/bin/bash # Search for any remaining TransferrableOwnership references rg "TransferrableOwnership" # Search for WithdrawablePeriphery usage rg "WithdrawablePeriphery" # Search for inheritance patterns ast-grep --pattern 'contract $_ is $$$TransferrableOwnership$$$' ast-grep --pattern 'contract $_ is $$$WithdrawablePeriphery$$$'Length of output: 65723
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
33-33: LGTM: Executor initialization updated with owner parameter.The constructor call has been properly updated to match the new Executor contract signature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (1)
83-96: LGTM: Error handling updates are consistent.The test has been correctly updated to use the new method name and standardized error message. Consider updating the error documentation to reflect this standardized error handling approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/Receiver.md(0 hunks)docs/ReceiverAcrossV3.md(0 hunks)docs/ReceiverStargateV2.md(0 hunks)src/Periphery/LiFiDEXAggregator.sol(3 hunks)src/Periphery/Receiver.sol(2 hunks)src/Periphery/ReceiverStargateV2.sol(3 hunks)test/solidity/Periphery/ReceiverAcrossV3.t.sol(3 hunks)test/solidity/Periphery/ReceiverStargateV2.t.sol(4 hunks)
💤 Files with no reviewable changes (3)
- docs/ReceiverAcrossV3.md
- docs/Receiver.md
- docs/ReceiverStargateV2.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/solidity/Periphery/ReceiverStargateV2.t.sol
🧰 Additional context used
📓 Learnings (1)
src/Periphery/LiFiDEXAggregator.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (13)
test/solidity/Periphery/ReceiverAcrossV3.t.sol (3)
33-33: LGTM: Constructor update aligns with ownership changes.The additional owner parameter in the Executor constructor correctly implements the ownership management refactoring.
64-64: Update test function names to match new method names.The test method names still reference "pull" while testing the
withdrawTokenfunctionality.Apply these changes:
- function test_OwnerCanPullERC20Token() public { + function test_OwnerCanWithdrawERC20Token() public { - function test_OwnerCanPullNativeToken() public { + function test_OwnerCanWithdrawNativeToken() public {Also applies to: 78-78
100-103: LGTM: Non-owner test correctly updated.The test properly verifies that unauthorized users cannot call
withdrawToken.src/Periphery/ReceiverStargateV2.sol (3)
11-11: LGTM! Import of WithdrawablePeriphery.The import statement is correctly placed and aligns with the contract's new inheritance structure.
38-38: Version bump and inheritance change look good.The version bump from 1.0.1 to 1.1.0 correctly reflects the change in functionality with the new inheritance structure. The change to inherit from
WithdrawablePeripheryaligns with the broader refactoring effort across the codebase.Also applies to: 41-41
67-67: Constructor initialization is correct.The constructor properly initializes the
WithdrawablePeripherycontract with the owner parameter.src/Periphery/Receiver.sol (3)
11-11: LGTM! Import of WithdrawablePeriphery.The import statement is correctly placed and aligns with the contract's new inheritance structure.
17-18: Version bump and inheritance change look good.The version bump from 2.0.3 to 2.1.0 correctly reflects the change in functionality with the new inheritance structure. The change to inherit from
WithdrawablePeripheryaligns with the broader refactoring effort across the codebase.
54-54: Constructor initialization is correct.The constructor properly initializes the
WithdrawablePeripherycontract with the owner parameter.src/Periphery/LiFiDEXAggregator.sol (4)
6-6: LGTM! Import of WithdrawablePeriphery.The import statement is correctly placed and aligns with the contract's new inheritance structure.
26-27: Version bump and inheritance change look good.The version bump from 1.5.1 to 1.6.0 correctly reflects the change in functionality with the new inheritance structure. The change to inherit from
WithdrawablePeripheryaligns with the broader refactoring effort across the codebase.
67-71: Constructor parameter addition is correct.The constructor has been properly updated to include the
_ownerparameter and initializeWithdrawablePeriphery. The initialization order and parameter passing look correct.
61-61: Verify the owner access check in the modifier.The modifier has been updated to use
ownerinstead ofowner(), which is correct for the newWithdrawablePeripheryinheritance. However, ensure that all privileged operations are properly restricted.Run the following script to verify privileged operations:
✅ Verification successful
Privileged operations are properly secured with correct owner access checks.
- Owner-only operations: setPriviledge()
- Owner/privileged user operations: pause(), resume()
- Owner state variable from WithdrawablePeriphery is correctly used in access checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for privileged operations and their access controls # Look for functions with onlyOwner or onlyOwnerOrPriviledgedUser modifiers rg -A 2 "onlyOwner|onlyOwnerOrPriviledgedUser"Length of output: 70722
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)