feat: fix rate limit for transferTokenLayerZero (DEV-1131)#227
feat: fix rate limit for transferTokenLayerZero (DEV-1131)#227deluca-mike wants to merge 5 commits intodevfrom
transferTokenLayerZero (DEV-1131)#227Conversation
Summary by OctaneNew ContractsNo new contracts were added. Updated Contracts
🔗 Commit Hash: 11c1cd0 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR modifies the LayerZero token transfer library to calculate rate limits based on actual post-transfer balance deltas instead of pre-call estimates, introduces a helper function to extract quoted amounts, and updates corresponding tests to reflect the new rate limiting behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Proxy
participant RateLimiter
participant Token
rect rgba(100, 150, 200, 0.5)
Note over Caller,Token: Old Flow: Pre-Transfer Rate Check
Caller->>RateLimiter: Check rate limit (pre-call)
RateLimiter-->>Caller: Verify limit not exceeded
Caller->>Proxy: Execute transfer
Proxy->>Token: Transfer tokens
end
rect rgba(150, 200, 100, 0.5)
Note over Caller,Token: New Flow: Post-Transfer Balance Delta
Caller->>Proxy: Capture balance before
Caller->>Proxy: Execute transfer
Proxy->>Token: Transfer tokens
Token-->>Proxy: Confirm transfer
Caller->>Proxy: Calculate delta (before - after)
Caller->>RateLimiter: Apply rate limit with actual delta
RateLimiter-->>Caller: Verify limit not exceeded
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Fix all issues with AI agents
In `@src/libraries/LayerZeroLib.sol`:
- Around line 57-75: The subtraction of balances can underflow if balanceAfter >
balanceBefore; compute balanceAfter =
IERC20Like(token).balanceOf(address(proxy)) after the proxy.doCallWithValue
call, then compute uint256 delta = balanceBefore > balanceAfter ? balanceBefore
- balanceAfter : 0 and pass delta into _rateLimited; update the call site that
currently computes balanceBefore - IERC20Like(token).balanceOf(address(proxy))
(involving IERC20Like, proxy.doCallWithValue, _rateLimited,
LIMIT_LAYERZERO_TRANSFER, oftAddress, destinationEndpointId, token) to use the
clamped delta instead to avoid revert on rebasing/reflective tokens.
- Around line 33-57: Add a guard after retrieving the token from
ILayerZero(oftAddress).token() to ensure token is not the zero address; if token
== address(0) revert with a clear error like "LayerZeroLib/invalid-token" so
subsequent calls (ApproveLib.approve(...), IERC20Like(token).balanceOf(...), and
any ERC20 interactions) cannot revert unexpectedly; update the code around the
token variable in LayerZeroLib so the require check runs immediately after
calling ILayerZero.token() and before any approve or balanceOf usage.
In `@test/mainnet-fork/LayerZero.t.sol`:
- Around line 206-212: The event declaration for OFTSent is syntactically
invalid due to a missing comma after the dstEid parameter; update the OFTSent
event signature (event OFTSent(... dstEid ... , address indexed fromAddress,
...)) by inserting the comma after dstEid so the parameter list is properly
comma-separated, then recompile to verify the error is resolved.
| address token = ILayerZero(oftAddress).token(); | ||
|
|
||
| // NOTE: Full integration testing of this logic is not possible without OFTs with | ||
| // approvalRequired == false. Add integration testing for this case before | ||
| // using in production. | ||
| if (ILayerZero(oftAddress).approvalRequired()) { | ||
| ApproveLib.approve( | ||
| ILayerZero(oftAddress).token(), | ||
| address(proxy), | ||
| oftAddress, | ||
| amount | ||
| ); | ||
| ApproveLib.approve(token, address(proxy), oftAddress, amount); | ||
| } | ||
|
|
||
| bytes memory options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(200_000, 0); | ||
|
|
||
| SendParam memory sendParams = SendParam({ | ||
| dstEid : destinationEndpointId, | ||
| to : layerZeroRecipient, | ||
| amountLD : amount, | ||
| minAmountLD : 0, | ||
| extraOptions : options, | ||
| extraOptions : OptionsBuilder.newOptions().addExecutorLzReceiveOption(200_000, 0), | ||
| composeMsg : "", | ||
| oftCmd : "" | ||
| }); | ||
|
|
||
| // Query the min amount received on the destination chain and set it. | ||
| ( , , OFTReceipt memory receipt ) = ILayerZero(oftAddress).quoteOFT(sendParams); | ||
| sendParams.minAmountLD = receipt.amountReceivedLD; | ||
| sendParams.minAmountLD = _getQuotedAmountReceivedLD(oftAddress, sendParams); | ||
|
|
||
| MessagingFee memory fee = ILayerZero(oftAddress).quoteSend(sendParams, false); | ||
|
|
||
| uint256 balanceBefore = IERC20Like(token).balanceOf(address(proxy)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
LayerZero OFT token() function return value address zero
💡 Result:
In LayerZero’s OFT (V2 / Endpoint V2) contracts, token() is meant to return the underlying ERC20 address:
OFT.sol(a “native” OFT that is the ERC20):token()returnsaddress(this)(the OFT contract address). [1]OFTAdapter.sol(wraps an existing ERC20):token()returns the adapted ERC20 (innerToken). [2]- LayerZero’s own contract playground also labels
token()as “Get Underlying Token Address”. [3]
So if you’re seeing address(0), it’s not expected for a standard OFT or OFTAdapter. The usual causes are:
- Wrong contract / wrong inheritance: you’re calling
token()on something that isn’tOFT/OFTAdapter(or a custom contract that didn’t overridetoken()as those do). - ABI mismatch / wrong address: your interface/ABI doesn’t match the deployed bytecode you’re querying (common when mixing OFT packages/versions), or you’re querying the wrong deployed address.
To debug quickly: verify the contract source on the explorer and confirm it is @layerzerolabs/oft-evm OFT or OFTAdapter, then call approvalRequired()—it’s typically false for OFT and true for the default OFTAdapter, which helps confirm what you’re interacting with. [3]
Sources: [1] [2] [3]
🌐 Web query:
LayerZero OFTAdapter OFT token() API contract documentation
💡 Result:
LayerZero OFTAdapter.token() API (EVM, V2 OFT)
Signature
function token() public view returns (address)
What it returns
- In
OFTAdapter,token()returns the underlying (adapted) ERC‑20 token address (address(innerToken)). This is explicitly because, for an adapter,address(this)(the adapter) is not the ERC‑20 token contract. [1]
Contrast with OFT (non-adapter)
- In
OFT,token()returnsaddress(this)(because the OFT contract is the ERC‑20). [2]
Where this fits conceptually
- LayerZero’s docs describe
OFTAdapteras the approach for existing ERC‑20s (typically lock/unlock semantics) without modifying the original token contract. [3][4]
Sources
- Verified
OFTAdapterimplementation showingtoken()returnsaddress(innerToken)(Etherscan). [1] OFTimplementation snippet showingtoken()returnsaddress(this)(layerzero.org). [2]- LayerZero V2 OFT Quickstart (OFT vs OFTAdapter overview). [3]
- OFT Technical Reference (adapter pattern explanation). [4]
🏁 Script executed:
# Search for LayerZero interface definitions in the repository
fd -e sol | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 647
🏁 Script executed:
# Search for ILayerZero interface definition
rg "interface ILayerZero" -A 20Repository: sparkdotfi/spark-alm-controller
Length of output: 1385
🏁 Script executed:
# Search for LayerZero OFT token() related code
rg "\.token\(\)" --type sol -B 2 -A 2Repository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
# Look at the LayerZeroLib.sol file to understand context
cat -n src/libraries/LayerZeroLib.sol | head -100Repository: sparkdotfi/spark-alm-controller
Length of output: 4510
🏁 Script executed:
# Search for where LayerZeroLib is used and with what OFT implementations
rg "LayerZeroLib" -A 5 -B 5 --type solRepository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
# Check for token address validation patterns
rg "token.*address\(0\)" --type solRepository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
# Search for OFT instantiation or deployment in tests/scripts
rg "new OFT\|OFTAdapter\|token()" --type sol -B 2 -A 2 | head -100Repository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
# Check if there are any checks for address(0) in the codebase
rg "== address\(0\)\|!= address\(0\)" --type sol | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
# Search for ApproveLib to understand what it does
cat -n src/libraries/ApproveLib.sol | head -50Repository: sparkdotfi/spark-alm-controller
Length of output: 1770
🏁 Script executed:
# Search for how oftAddress is validated
rg "oftAddress" --type-listRepository: sparkdotfi/spark-alm-controller
Length of output: 464
🏁 Script executed:
# Use rg without type filter
rg "oftAddress" -B 3 -A 3 | head -150Repository: sparkdotfi/spark-alm-controller
Length of output: 7080
🏁 Script executed:
# Search for any address(0) checks in the codebase
rg "address\(0\)" | head -30Repository: sparkdotfi/spark-alm-controller
Length of output: 2952
🏁 Script executed:
# Check the imports and understand IERC20Like
rg "IERC20Like" -B 2 -A 5Repository: sparkdotfi/spark-alm-controller
Length of output: 9084
Add a guard against token == address(0) to prevent reverts in ERC20 calls.
While standard LayerZero OFT and OFTAdapter implementations never return address(0) from token(), the interface provides no guarantee. If an OFT variant or misconfigured contract returns zero, the subsequent approve() and balanceOf() calls will revert. Add a require statement at line 33 to validate the token address:
address token = ILayerZero(oftAddress).token();
require(token != address(0), "LayerZeroLib/invalid-token");Do not fall back to oftAddress as suggested—for OFT contracts, token() returns address(this) (the OFT itself), and for OFTAdapter, the fallback would be incorrect.
🤖 Prompt for AI Agents
In `@src/libraries/LayerZeroLib.sol` around lines 33 - 57, Add a guard after
retrieving the token from ILayerZero(oftAddress).token() to ensure token is not
the zero address; if token == address(0) revert with a clear error like
"LayerZeroLib/invalid-token" so subsequent calls (ApproveLib.approve(...),
IERC20Like(token).balanceOf(...), and any ERC20 interactions) cannot revert
unexpectedly; update the code around the token variable in LayerZeroLib so the
require check runs immediately after calling ILayerZero.token() and before any
approve or balanceOf usage.
Overview
🔗 Commit Hash: 11c1cd0 |
lucas-manuel
added
Status: Revisions Needed
Status: Merge Conflicts
and removed
Status: Ready for Review
labels
lucas-manuel
changed the title
transferTokenLayerZerotransferTokenLayerZero (DEV-1131)
deluca-mike
force-pushed
the
fix/rate-limit-for-transferTokenLayerZero
branch
from
11c1cd0 to
7e7bf7c
Compare
deluca-mike
added
Status: Ready for Review
and removed
Status: Merge Conflicts
Status: Revisions Needed
labels
src/libraries/LayerZeroLib.sol
Outdated
| ), | ||
| amount | ||
| ); | ||
| require(layerZeroRecipient != bytes32(0), "MC/recipient-not-set"); |
There was a problem hiding this comment.
@lucas-manuel None of our libs use the lib name in the error message. Is that what we what to adopt going forward?
There was a problem hiding this comment.
This is the first lib thats being used by both controllers, so yeah this one can first then the rest
| 10_000_000e6 + 1, | ||
| destinationEndpointId | ||
| ); | ||
|
|
| vm.expectRevert("LayerZeroLib/recipient-not-set"); | ||
| foreignController.transferTokenLayerZero{value: fee.nativeFee}( | ||
| USDT_OFT, | ||
| 10_000_000e6, |
There was a problem hiding this comment.
Too many extra lines after last function bracket and last contract brancket
There was a problem hiding this comment.
I don't see where.
|
Coverage after merging fix/rate-limit-for-transferTokenLayerZero into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.