Conversation
…sset for ERC20 transfers; version bumps for both contracts
… function names for clarity
…larity and consistency
lifi-action-bot
changed the title
WalkthroughConsolidates ERC20 transfers/approvals to LibAsset helpers across facets and periphery, special-cases Tron USDT in LibAsset with new constants and tests, tightens Solidity test naming rules, updates multiple tests to expect ETHTransferFailed, adds MockTronUSDT test-double, and bumps several Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Coverage ReportLine Coverage: 88.03% (3223 / 3661 lines) |
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/solidity/Libraries/LibAsset.t.sol (1)
197-223: Align the last two test names with the rule you just tightened.
test_TronUsdtIsPresentAtCanonicalAddressAfterLocalStubandtest_StablecoinTransferSucceedsTronChainIdstill lean on implementation detail / vague nouns more than the surrounding renamed tests. Renaming them to behavior-first names would avoid leaving mixed styles in the same file.As per coding guidelines, "Avoid stuffing names with test mechanics ... unless two tests would otherwise be indistinguishable" and "When editing any
test/**/*.t.solfile, align existing test names in that file to this convention if they are still short or ambiguous."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Libraries/LibAsset.t.sol` around lines 197 - 223, Two tests use implementation-detail names; rename function test_TronUsdtIsPresentAtCanonicalAddressAfterLocalStub and test_StablecoinTransferSucceedsTronChainId to behavior-first, descriptive names. Update the declarations and any references so they describe observable behavior (e.g., presence/availability and successful transfer on Tron chain) rather than stubbing/chain-id mechanics; modify function names for the functions test_TronUsdtIsPresentAtCanonicalAddressAfterLocalStub and test_StablecoinTransferSucceedsTronChainId accordingly and keep assertions and body unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Helpers/WithdrawablePeriphery.sol`:
- Around line 2-3: Remove the stray `@custom:version` tag placed above the
pragma and instead add it into the contract's NatSpec block for
WithdrawablePeriphery (e.g., alongside `@title`) so the contract header includes
`@custom:version 1.0.1`; also ensure the file retains the SPDX comment followed
immediately by pragma with no blank line between them.
---
Nitpick comments:
In `@test/solidity/Libraries/LibAsset.t.sol`:
- Around line 197-223: Two tests use implementation-detail names; rename
function test_TronUsdtIsPresentAtCanonicalAddressAfterLocalStub and
test_StablecoinTransferSucceedsTronChainId to behavior-first, descriptive names.
Update the declarations and any references so they describe observable behavior
(e.g., presence/availability and successful transfer on Tron chain) rather than
stubbing/chain-id mechanics; modify function names for the functions
test_TronUsdtIsPresentAtCanonicalAddressAfterLocalStub and
test_StablecoinTransferSucceedsTronChainId accordingly and keep assertions and
body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8796d4cc-f146-42cc-9d5f-95e93dbd54e6
📒 Files selected for processing (7)
.cursor/rules/400-solidity-tests.mdcsrc/Facets/GenericSwapFacetV3.solsrc/Helpers/WithdrawablePeriphery.solsrc/Libraries/LibAsset.solsrc/Periphery/LiFiDEXAggregator.soltest/solidity/Libraries/LibAsset.t.soltest/solidity/utils/MockTronUSDT.sol
… to handle TRON USDT
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Facets/GenericSwapFacetV3.sol (1)
9-9: Minor: Import ordering convention.Per coding guidelines, external library imports should be grouped before local imports. The OpenZeppelin
IERC20import could be moved above the localILiFiimport. This is a minor stylistic note and doesn't affect functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facets/GenericSwapFacetV3.sol` at line 9, The import ordering in GenericSwapFacetV3.sol should place external library imports before local project imports; move the OpenZeppelin IERC20 import so it appears above the local ILiFi import (i.e., ensure import { IERC20 } from "@openzeppelin/..."; is listed before import { ILiFi } ...), keeping the same symbols and no other changes to code or semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Facets/GenericSwapFacetV3.sol`:
- Line 9: The import ordering in GenericSwapFacetV3.sol should place external
library imports before local project imports; move the OpenZeppelin IERC20
import so it appears above the local ILiFi import (i.e., ensure import { IERC20
} from "@openzeppelin/..."; is listed before import { ILiFi } ...), keeping the
same symbols and no other changes to code or semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfc5053d-a236-4515-9b45-f6a284908aa5
📒 Files selected for processing (8)
.cursor/rules/100-solidity-basics.mdcsrc/Facets/GenericSwapFacetV3.solsrc/Helpers/WithdrawablePeriphery.soltest/solidity/Helpers/WithdrawablePeriphery.t.soltest/solidity/Periphery/Across/V3/ReceiverAcrossV3.t.soltest/solidity/Periphery/Across/V4/ReceiverAcrossV4.t.soltest/solidity/Periphery/ReceiverOIF.t.soltest/solidity/Periphery/ReceiverStargateV2.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Helpers/WithdrawablePeriphery.sol
lifi-action-bot
changed the title
lifi-action-bot
changed the title
…ude rules for handling errors in GenericErrors.sol
| /// @dev Can only execute calldata for whitelisted function selectors | ||
| /// @custom:version 1.0.2 | ||
| /// @custom:version 2.0.0 | ||
| contract GenericSwapFacetV3 is ILiFi { |
There was a problem hiding this comment.
I chose to bump the version instead of introducing a new GenericSwapFacetV4, since the changes are purely internal. All function selectors remain unchanged, so there’s no need to maintain a legacy version of GenericSwapFacetV3.
| } else { | ||
| assetId.safeTransfer(receiver, amount); | ||
| } | ||
| if (amount == 0) revert ZeroAmount(); |
There was a problem hiding this comment.
It’s an admin-only function, so removing it and introducing a new error won’t impact any of our offchain indexers or scripts. @0xDEnYO I’d like to double check this with you
mirooon
changed the title
lifi-action-bot
changed the title
3 participants
Which Linear task belongs to this PR?
https://linear.app/lifi-linear/issue/EXSC-241/fix-tron-usdt-transfers-failing-on-safetransferlib-return-data-check
Why did I implement it this way?
Problem
Tron’s canonical USDT contract
(
TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t/0xa614f803B6FD780986A42c78Ec9c7f77e6DeD13C)was compiled with an older Solidity version (~0.4.x).
Although its
transfer()function is declared asreturns (bool), it does not explicitly returntrue. As a result:The transfer executes successfully and balances update correctly.
However, the return data contains 32 zero bytes (
false) instead of:true(standard ERC20 behavior).This creates incompatibility with
SafeTransferLib.safeTransfer, which enforces strict return validation:Accepts:
returndatasize() == 0, or1Rejects:
0→ reverts withTransferFailedImpact:
All outbound
transfer()calls to Tron USDT reverted inside our contracts (viaLibAsset.transferERC20→SafeTransferLib.safeTransfer), even though the transfer itself succeeded.Important:
transferFrom()is not affected — Tron USDT correctly returnstrue.Inbound flows using
transferFrom()remain safe.Solution
A targeted exception was introduced in
LibAsset.transferERC20:Detect:
chainId = 728126428)In this specific case:
IERC20.transfer()callSafeTransferLibreturn-data validationStill:
All other tokens and chains continue to use
SafeTransferLib.safeTransferunchanged.What Was Done
Core Fix
src/Libraries/LibAsset.sol(v2.2.0)TRON_CHAIN_IDandTRON_USDTconstantstransferERC20to apply the bypass only for Tron USDTBreaking Changes
src/Facets/GenericSwapFacetV3.sol(v2.0.0)Main changes:
The migration to
LibAsset.transferERC20ensures the Tron USDT fix is applied consistently.This release introduces a breaking security model change. As this update already required changes to GenericSwapFacetV3, I used the opportunity to consolidate improvements and minimize audit overhead by batching them into a single audit cycle.
As part of this, the facet was migrated to the new whitelisting model:
Additional changes:
APPROVE_TO_ONLY_SELECTOR = 0xffffffffsentinel for approve-only targetssrc/Helpers/WithdrawablePeriphery.sol(v2.0.0)Two breaking changes:
if (amount == 0) revert ZeroAmount()ExternalCallFailedETHTransferFailedon native transfer failureOther Contract Updates
src/Periphery/LiFiDEXAggregator.sol(v1.13.0)Migrated all ERC20
transfer/transferFromcalls to:LibAsset.transferERC20LibAsset.transferFromERC20src/Helpers/WithdrawablePeriphery.solwithdrawTokennow usesLibAsset.transferERC20Tests & Tooling
Added
MockTronUSDTcontract reproducing the faulty return behavior (transfer()returnsfalse)Added Tron-specific
LibAssettest suite:SafeTransferLibUpdated multiple receiver test files due to error selector changes
Added/updated internal guidelines:
.cursor/rules/100-solidity-basics.mdc.cursor/rules/400-solidity-tests.mdcContracts Not Modified
src/Periphery/TokenWrapper.solsrc/Periphery/LidoWrapper.solReceiver Contracts
ReceiverAcrossV3.solReceiverChainflip.solReceiverStargateV2.solNot modified because:
Note:
If deployed on Tron in the future:
transfer()must route throughLibAsset.transferERC20transferFrom()usage remains safe (returnstrueon Tron USDT)Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)