Add parameter validation to allbridge lf 14440 #1275
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ter-validation-to-allbridge-lf-14440
Contributor
WalkthroughA new helper contract Changes
Possibly related PRs
Suggested labels
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
0xDEnYO
commented
Jul 14, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Errors/GenericErrors.sol (1)
2-2: Update the version tag to reflect the new error addition.Since a new error
InvalidNonEVMReceiverwas added, the custom:version tag should be incremented.Apply this diff to update the version:
-/// @custom:version 1.0.1 +/// @custom:version 1.0.2Also applies to: 22-22
🧹 Nitpick comments (2)
src/Facets/AllBridgeFacet.sol (1)
190-192: Clarify the documentation for chain ID conversion.The comment states "Converts LiFi internal chain IDs to AllBridge chain IDs", but many of these are standard EVM chain IDs (e.g., 1 for Ethereum, 137 for Polygon). Consider updating the documentation for clarity.
Apply this diff to improve the documentation:
- /// @notice Converts LiFi internal chain IDs to AllBridge chain IDs + /// @notice Converts standard blockchain chain IDs to AllBridge-specific chain IDs /// @param destinationChainId The LiFi chain ID to convert - /// @return The corresponding Chainflip chain ID + /// @return The corresponding AllBridge chain IDtest/solidity/Facets/AllBridgeFacet.t.sol (1)
30-53: Consider removing duplicate LIFI_CHAIN_ID constants.Since the test contract inherits from
LiFiData, theLIFI_CHAIN_ID_*constants (lines 46-53) are already available and don't need to be redeclared. TheALLBRIDGE_ID_*constants should remain as they're not inLiFiData.Apply this diff to remove the duplicate constants:
contract AllBridgeFacetTest is TestBaseFacet, LiFiData { IAllBridge internal constant ALLBRIDGE_ROUTER = IAllBridge(0x609c690e8F7D68a59885c9132e812eEbDaAf0c9e); address internal constant ALLBRIDGE_POOL = 0xa7062bbA94c91d565Ae33B893Ab5dFAF1Fc57C4d; uint32 private constant ALLBRIDGE_ID_ETHEREUM = 1; uint32 private constant ALLBRIDGE_ID_BSC = 2; uint32 private constant ALLBRIDGE_ID_TRON = 3; uint32 private constant ALLBRIDGE_ID_SOLANA = 4; uint32 private constant ALLBRIDGE_ID_POLYGON = 5; uint32 private constant ALLBRIDGE_ID_ARBITRUM = 6; uint32 private constant ALLBRIDGE_ID_AVALANCHE = 8; uint32 private constant ALLBRIDGE_ID_BASE = 9; uint32 private constant ALLBRIDGE_ID_OPTIMISM = 10; uint32 private constant ALLBRIDGE_ID_CELO = 11; uint32 private constant ALLBRIDGE_ID_SUI = 13; - uint256 internal constant LIFI_CHAIN_ID_ETHEREUM = 1; - uint256 internal constant LIFI_CHAIN_ID_ARBITRUM = 42161; - uint256 internal constant LIFI_CHAIN_ID_AVALANCHE = 43114; - uint256 internal constant LIFI_CHAIN_ID_BASE = 8453; - uint256 internal constant LIFI_CHAIN_ID_BSC = 56; - uint256 internal constant LIFI_CHAIN_ID_CELO = 42220; - uint256 internal constant LIFI_CHAIN_ID_OPTIMISM = 10; - uint256 internal constant LIFI_CHAIN_ID_POLYGON = 137; error UnsupportedAllBridgeChainId();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Errors/GenericErrors.sol(1 hunks)src/Facets/AllBridgeFacet.sol(6 hunks)src/Helpers/LiFiData.sol(1 hunks)test/solidity/Facets/AllBridgeFacet.t.sol(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
src/Facets/*Facet.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1169
File: script/deploy/zksync/DeployFeeCollector.zksync.s.sol:32-37
Timestamp: 2025-05-27T12:00:43.940Z
Learning: The lifinance/contracts repository has deployment scripts that perform validation checks (including zero-address validation) before executing individual deploy scripts, making runtime validation checks in the deploy scripts themselves redundant.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: lifinance/contracts#963
File: script/deploy/zksync/DeploySymbiosisFacet.zksync.s.sol:21-34
Timestamp: 2025-01-30T02:51:43.006Z
Learning: Parameter validation for deployment scripts in the LiFi contracts is handled by pre-deployment scripts, making additional validation within individual deployment scripts unnecessary.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1102
File: script/tasks/diamondSyncSigs_FAST.sh:14-18
Timestamp: 2025-04-23T05:20:45.831Z
Learning: In the lifinance/contracts repository, the `diamondSyncSigs_FAST` function in script/tasks/diamondSyncSigs_FAST.sh doesn't need explicit parameter validation (like for the ENVIRONMENT parameter) because it's called from a master script that ensures these parameters are available.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: mirooon
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:63-64
Timestamp: 2025-02-26T13:25:23.615Z
Learning: In the LiFi contracts repository, RPC URL validation is handled in the `getViemChainForNetworkName` helper function, which throws a descriptive error if the RPC URL is missing. This ensures that by the time the chain object is used elsewhere in the codebase, a valid RPC URL is guaranteed to exist.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: deployments/_deployments_log_file.json:34037-34080
Timestamp: 2025-06-13T08:30:26.220Z
Learning: LiFiDiamond contains generic withdrawal logic, so individual facet contracts (e.g., PioneerFacet) do not need their own Ether-withdraw functions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: mirooon
PR: lifinance/contracts#945
File: script/demoScripts/demoGlacis.ts:180-189
Timestamp: 2025-01-30T10:38:18.041Z
Learning: The executeTransaction helper function in the LiFi codebase handles error cases and transaction logging comprehensively, including try/catch structures, transaction receipt validation, and detailed console logging for transaction status tracking.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
src/Errors/GenericErrors.sol (10)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For transactions targeting non-EVM chains, receiverAddress must be declared as bytes (not bytes32), and the facet must ensure that the non-EVM address is not zero, reverting with InvalidNonEVMReceiver if so. When a non-EVM address is used, bridgeData.receiver must contain LibAsset.NON_EVM_ADDRESS.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/boba.diamond.json:68-68
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, `ReceiverStargateV2` may not be deployed to all networks, and it's acceptable for its address to remain empty in deployment files when it is not deployed. PRs unrelated to `ReceiverStargateV2` should not raise comments about its absence.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/boba.diamond.json:68-68
Timestamp: 2024-10-04T09:10:17.997Z
Learning: In the `lifinance/contracts` repository, `ReceiverStargateV2` may not be deployed to all networks, and it's acceptable for its address to remain empty in deployment files when it is not deployed. PRs unrelated to `ReceiverStargateV2` should not raise comments about its absence.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/**/*.sol : Enforce the use of custom errors to save gas during error handling (gas-custom-errors).
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/**/*.sol : Validate constructor inputs rigorously: if an invalid value (e.g., address(0) or zero value) is provided, revert with a custom error such as InvalidConfig. Ensure tests cover these conditions.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.
src/Helpers/LiFiData.sol (15)
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#950
File: script/demoScripts/utils/demoScriptChainConfig.ts:17-20
Timestamp: 2025-01-24T14:53:20.703Z
Learning: In the LiFi contracts repository, RPC URLs should be strictly sourced from environment variables without fallback to networks.json configuration.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1029
File: deployments/_deployments_log_file.json:25831-25847
Timestamp: 2025-02-25T10:37:22.380Z
Learning: In the LiFi contracts repository, deployment log files (like _deployments_log_file.json) are organized by contract type, then by network name, then by environment and version. The same network name (e.g., "sonic") can appear multiple times under different contract types, which is the expected structure and not an issue.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-04T09:06:19.927Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: ezynda3
PR: lifinance/contracts#1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:119-131
Timestamp: 2025-02-13T08:54:06.846Z
Learning: In Solidity, when mapping between fixed values known at compile time (like chain IDs), using if-else statements with constants is preferred over mappings since mappings cannot be declared as constant or immutable.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:21-23
Timestamp: 2025-05-14T16:28:30.376Z
Learning: When shifting by 16 bits per chain ID in the GasZip contracts, the MAX_CHAINID_LENGTH_ALLOWED constant should be set to 16 (since a uint256 can hold at most 16 chain IDs when using 16 bits per chain). This constant should be consistent between related contracts like GasZipPeriphery and GasZipFacet.
Learnt from: ezynda3
PR: lifinance/contracts#806
File: deployments/_deployments_log_file.json:5780-5793
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `DiamondCutFacet` address `0xd5cf40a2a18b633cfd6a1ae16d1771596498cf83` in the `LiFiDiamond` deployment on `xlayer` are correct and should not be flagged as issues, even if they are not referenced in the Solidity files.
Learnt from: ezynda3
PR: lifinance/contracts#909
File: script/deploy/facets/DeployTokenWrapper.s.sol:43-49
Timestamp: 2025-01-09T04:34:17.861Z
Learning: Zero address validation is not required for configuration values extracted from config files in the LiFi contracts codebase, as these files are considered trusted sources.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
src/Facets/AllBridgeFacet.sol (34)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must always include the following three functions: _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts typically inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, Validatable, and may use ECDSA.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Each facet must properly use the following modifiers: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps / doesContainSourceSwaps, doesNotContainDestinationCalls / doesContainDestinationCalls.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For transactions targeting non-EVM chains, receiverAddress must be declared as bytes (not bytes32), and the facet must ensure that the non-EVM address is not zero, reverting with InvalidNonEVMReceiver if so. When a non-EVM address is used, bridgeData.receiver must contain LibAsset.NON_EVM_ADDRESS.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facet contract names must include the word Facet.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: ezynda3
PR: lifinance/contracts#1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-04T09:00:06.744Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: script/deploy/facets/DeployLiFiDEXAggregator.s.sol:33-35
Timestamp: 2025-01-21T11:07:36.236Z
Learning: In the LiFiDEXAggregator contract deployment (DeployLiFiDEXAggregator.s.sol), the refundWallet address from global.json is used as the owner parameter in the contract's constructor, which is then passed to the WithdrawablePeriphery base contract.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For facets with a receiverAddress parameter, it should be the first parameter in the {facetName}Data struct. The receiverAddress must always be explicitly validated against bridgeData.receiver in the function logic.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/**/*.sol : Validate constructor inputs rigorously: if an invalid value (e.g., address(0) or zero value) is provided, revert with a custom error such as InvalidConfig. Ensure tests cover these conditions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For native fees, the facet must use the _depositAndSwap variant that reserves the required fee before execution.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:21-23
Timestamp: 2025-05-14T16:28:30.376Z
Learning: When shifting by 16 bits per chain ID in the GasZip contracts, the MAX_CHAINID_LENGTH_ALLOWED constant should be set to 16 (since a uint256 can hold at most 16 chain IDs when using 16 bits per chain). This constant should be consistent between related contracts like GasZipPeriphery and GasZipFacet.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:119-131
Timestamp: 2025-02-13T08:54:06.846Z
Learning: In Solidity, when mapping between fixed values known at compile time (like chain IDs), using if-else statements with constants is preferred over mappings since mappings cannot be declared as constant or immutable.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
test/solidity/Facets/AllBridgeFacet.t.sol (30)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must always include the following three functions: _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to tests/**/*.t.sol : Any contract that inherits from TestBase.sol must call initTestBase() in setUp() and set facet address.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For transactions targeting non-EVM chains, receiverAddress must be declared as bytes (not bytes32), and the facet must ensure that the non-EVM address is not zero, reverting with InvalidNonEVMReceiver if so. When a non-EVM address is used, bridgeData.receiver must contain LibAsset.NON_EVM_ADDRESS.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts typically inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, Validatable, and may use ECDSA.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Each facet must properly use the following modifiers: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps / doesContainSourceSwaps, doesNotContainDestinationCalls / doesContainDestinationCalls.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For facets with a receiverAddress parameter, it should be the first parameter in the {facetName}Data struct. The receiverAddress must always be explicitly validated against bridgeData.receiver in the function logic.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.377Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Learnt from: mirooon
PR: lifinance/contracts#1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:0-0
Timestamp: 2024-10-31T09:10:16.115Z
Learning: In the `signData` function within `RelayFacetTest` in `test/solidity/Facets/RelayFacet.t.sol`, the use of the EIP-712 standard for hashing and signing is not required.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:62-71
Timestamp: 2025-05-14T15:19:24.984Z
Learning: The syntax `bytes4(calldata_bytes[:4])` is valid in Solidity 0.8.x for extracting a function selector from calldata bytes.
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:153-181
Timestamp: 2025-07-11T09:48:47.195Z
Learning: In fork-based tests like AllowListMigratorFacet.t.sol, dynamically counting JSON array elements can cause EvmError: MemoryOOG due to memory constraints in the fork environment. Using hardcoded counts is acceptable when the array only grows (selectors are added but not removed), as the risk of mismatch is minimal.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:50-56
Timestamp: 2025-02-14T09:37:32.647Z
Learning: ChainflipData struct in ChainflipFacet.sol does not require additional field validation as confirmed by the team.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For native fees, the facet must use the _depositAndSwap variant that reserves the required fee before execution.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : After a swap, verify if facet-specific parameters (e.g., expected outputAmount) require adjustment based on the actual swap outcome.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to tests/**/*.t.sol : All tests that verify a successful execution must be prefixed with: test_. All tests that verify a failure case and expect a specific revert reason must be prefixed with: testRevert_. Negative tests must check for a specific revert reason.
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Errors/GenericErrors.sol
[error] 1-1: The file has relevant changes but the file version was not updated. Please update the custom:version tag in this contract.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: generate-tag
🔇 Additional comments (7)
src/Helpers/LiFiData.sol (1)
9-19: Well-structured helper contract.The contract properly centralizes chain ID constants and the NON_EVM_ADDRESS, which improves maintainability by avoiding duplicate definitions across facets.
src/Facets/AllBridgeFacet.sol (4)
72-76: Proper constructor validation added.Good addition of zero address validation for the allBridge parameter, following the coding guidelines for constructor input validation.
132-137: Correct validation of destination chain ID consistency.The validation ensures that the AllBridge-specific chain ID matches the mapped value from the LiFi chain ID, preventing mismatched destination chains.
138-151: Comprehensive receiver address validation.The implementation correctly handles both EVM and non-EVM receiver validation according to the facet requirements, ensuring proper address consistency and preventing invalid non-EVM receivers.
18-24: Excellent implementation of enhanced validation and chain ID mapping.The upgrade properly implements:
- Comprehensive parameter validation for chain IDs and receiver addresses
- Support for both EVM and non-EVM destinations
- Flexible fee payment options (native vs sending asset)
- Proper error handling with specific revert reasons
All facet requirements are met.
Also applies to: 129-188
test/solidity/Facets/AllBridgeFacet.t.sol (2)
140-258: Comprehensive test coverage for new validation logic.Excellent test coverage including:
- Constructor validation with zero address
- Destination chain ID mismatch scenarios
- Receiver address validation for both EVM and non-EVM cases
- All error cases with proper revert reasons
The tests follow the correct naming conventions and thoroughly validate the new functionality.
260-317: Thorough validation of chain ID mappings.The test comprehensively validates all supported chain ID mappings and properly tests the revert case for unsupported chains. This ensures the
_getAllBridgeChainIdfunction works correctly for all supported blockchains.
Collaborator
Test Coverage ReportLine Coverage: 84.40% (2273 / 2693 lines) |
…ter-validation-to-allbridge-lf-14440
Collaborator
🤖 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: ✅ No unresolved security alerts! 🎉 |
auto-merge was automatically disabled
July 15, 2025 03:31
Pull request was converted to draft
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Facets/AllBridgeFacet.sol(6 hunks)src/Helpers/LiFiData.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
src/Facets/*Facet.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1169
File: script/deploy/zksync/DeployFeeCollector.zksync.s.sol:32-37
Timestamp: 2025-05-27T12:00:43.940Z
Learning: The lifinance/contracts repository has deployment scripts that perform validation checks (including zero-address validation) before executing individual deploy scripts, making runtime validation checks in the deploy scripts themselves redundant.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: mirooon
PR: lifinance/contracts#1187
File: .github/workflows/versionControlAndAuditCheck.yml:596-612
Timestamp: 2025-06-05T11:25:43.443Z
Learning: The user mirooon has an internal system for tracking tickets and technical debt. They acknowledged the security concern about disabled commit hash verification in the audit workflow and plan to implement conditional logic to skip verification only for revert PRs in a future update.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: mirooon
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:63-64
Timestamp: 2025-02-26T13:25:23.615Z
Learning: In the LiFi contracts repository, RPC URL validation is handled in the `getViemChainForNetworkName` helper function, which throws a descriptive error if the RPC URL is missing. This ensures that by the time the chain object is used elsewhere in the codebase, a valid RPC URL is guaranteed to exist.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: deployments/_deployments_log_file.json:34037-34080
Timestamp: 2025-06-13T08:30:26.220Z
Learning: LiFiDiamond contains generic withdrawal logic, so individual facet contracts (e.g., PioneerFacet) do not need their own Ether-withdraw functions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: mirooon
PR: lifinance/contracts#945
File: script/demoScripts/demoGlacis.ts:180-189
Timestamp: 2025-01-30T10:38:18.041Z
Learning: The executeTransaction helper function in the LiFi codebase handles error cases and transaction logging comprehensively, including try/catch structures, transaction receipt validation, and detailed console logging for transaction status tracking.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: script/helperFunctions.sh:110-133
Timestamp: 2025-07-15T03:34:58.827Z
Learning: In script/helperFunctions.sh, 0xDEnYO prefers to keep the existing file locking implementation (using EXIT trap) unchanged even when potential improvements are suggested, prioritizing working code stability over refactoring when the current solution is functional.
src/Helpers/LiFiData.sol (22)
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: mirooon
PR: lifinance/contracts#950
File: script/demoScripts/utils/demoScriptChainConfig.ts:17-20
Timestamp: 2025-01-24T14:53:20.703Z
Learning: In the LiFi contracts repository, RPC URLs should be strictly sourced from environment variables without fallback to networks.json configuration.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-04T09:06:19.927Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: ezynda3
PR: lifinance/contracts#1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1029
File: deployments/_deployments_log_file.json:25831-25847
Timestamp: 2025-02-25T10:37:22.380Z
Learning: In the LiFi contracts repository, deployment log files (like _deployments_log_file.json) are organized by contract type, then by network name, then by environment and version. The same network name (e.g., "sonic") can appear multiple times under different contract types, which is the expected structure and not an issue.
Learnt from: ezynda3
PR: lifinance/contracts#806
File: deployments/_deployments_log_file.json:5780-5793
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `DiamondCutFacet` address `0xd5cf40a2a18b633cfd6a1ae16d1771596498cf83` in the `LiFiDiamond` deployment on `xlayer` are correct and should not be flagged as issues, even if they are not referenced in the Solidity files.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:21-23
Timestamp: 2025-05-14T16:28:30.376Z
Learning: When shifting by 16 bits per chain ID in the GasZip contracts, the MAX_CHAINID_LENGTH_ALLOWED constant should be set to 16 (since a uint256 can hold at most 16 chain IDs when using 16 bits per chain). This constant should be consistent between related contracts like GasZipPeriphery and GasZipFacet.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/boba.diamond.json:68-68
Timestamp: 2024-10-04T09:10:17.997Z
Learning: In the `lifinance/contracts` repository, `ReceiverStargateV2` may not be deployed to all networks, and it's acceptable for its address to remain empty in deployment files when it is not deployed. PRs unrelated to `ReceiverStargateV2` should not raise comments about its absence.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/boba.diamond.json:68-68
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, `ReceiverStargateV2` may not be deployed to all networks, and it's acceptable for its address to remain empty in deployment files when it is not deployed. PRs unrelated to `ReceiverStargateV2` should not raise comments about its absence.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: config/dexs.json:748-752
Timestamp: 2024-11-21T08:39:29.530Z
Learning: In the 'worldchain' network, the addresses `0x50D5a8aCFAe13Dceb217E9a071F6c6Bd5bDB4155`, `0x8f023b4193a6b18C227B4a755f8e28B3D30Ef9a1`, and `0x603a538477d44064eA5A5d8C345b4Ff6fca1142a` are used as DEXs and should be included in `config/dexs.json`.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:119-131
Timestamp: 2025-02-13T08:54:06.846Z
Learning: In Solidity, when mapping between fixed values known at compile time (like chain IDs), using if-else statements with constants is preferred over mappings since mappings cannot be declared as constant or immutable.
Learnt from: ezynda3
PR: lifinance/contracts#909
File: script/deploy/facets/DeployTokenWrapper.s.sol:43-49
Timestamp: 2025-01-09T04:34:17.861Z
Learning: Zero address validation is not required for configuration values extracted from config files in the LiFi contracts codebase, as these files are considered trusted sources.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
src/Facets/AllBridgeFacet.sol (34)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must always include the following three functions: _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts typically inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, Validatable, and may use ECDSA.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For transactions targeting non-EVM chains, receiverAddress must be declared as bytes (not bytes32), and the facet must ensure that the non-EVM address is not zero, reverting with InvalidNonEVMReceiver if so. When a non-EVM address is used, bridgeData.receiver must contain LibAsset.NON_EVM_ADDRESS.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Each facet must properly use the following modifiers: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps / doesContainSourceSwaps, doesNotContainDestinationCalls / doesContainDestinationCalls.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: ezynda3
PR: lifinance/contracts#1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-04T09:00:06.744Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: script/deploy/facets/DeployLiFiDEXAggregator.s.sol:33-35
Timestamp: 2025-01-21T11:07:36.236Z
Learning: In the LiFiDEXAggregator contract deployment (DeployLiFiDEXAggregator.s.sol), the refundWallet address from global.json is used as the owner parameter in the contract's constructor, which is then passed to the WithdrawablePeriphery base contract.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For facets with a receiverAddress parameter, it should be the first parameter in the {facetName}Data struct. The receiverAddress must always be explicitly validated against bridgeData.receiver in the function logic.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/**/*.sol : Validate constructor inputs rigorously: if an invalid value (e.g., address(0) or zero value) is provided, revert with a custom error such as InvalidConfig. Ensure tests cover these conditions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For native fees, the facet must use the _depositAndSwap variant that reserves the required fee before execution.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:21-23
Timestamp: 2025-05-14T16:28:30.376Z
Learning: When shifting by 16 bits per chain ID in the GasZip contracts, the MAX_CHAINID_LENGTH_ALLOWED constant should be set to 16 (since a uint256 can hold at most 16 chain IDs when using 16 bits per chain). This constant should be consistent between related contracts like GasZipPeriphery and GasZipFacet.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:119-131
Timestamp: 2025-02-13T08:54:06.846Z
Learning: In Solidity, when mapping between fixed values known at compile time (like chain IDs), using if-else statements with constants is preferred over mappings since mappings cannot be declared as constant or immutable.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
🪛 GitHub Actions: Security Alerts Review
src/Facets/AllBridgeFacet.sol
[error] 176-176: Olympix Integrated Security: Performing a narrowing downcast may result in silent overflow due to bit truncation.
[error] 164-164: Olympix Integrated Security: Performing a narrowing downcast may result in silent overflow due to bit truncation.
[error] 175-175: Olympix Integrated Security: Unchecked zero values passed as parameters may be in error or an indication of potentially insecure code design.
[error] 128-128: Olympix Integrated Security: Reentrant functions which emit events after making an external call may lead to out-of-order events.
🔇 Additional comments (7)
src/Helpers/LiFiData.sol (1)
1-22: LGTM! Well-structured helper contract for centralized configuration.The
LiFiDatacontract properly centralizes chain ID and address constants with correct naming conventions and valid non-zero chain IDs. This addresses the incomplete chain ID definitions mentioned in previous comments.src/Facets/AllBridgeFacet.sol (6)
12-13: Good integration with the new LiFiData helper contract.The new imports and inheritance properly integrate the centralized configuration data and error handling. The version bump to 2.1.0 appropriately reflects the enhanced functionality.
Also applies to: 18-25
26-45: Well-organized chain ID constants for mapping logic.The constants are properly defined and will be used for the explicit chain ID mapping functionality. The naming follows conventions and separates AllBridge IDs from LiFi chain IDs clearly.
72-75: Excellent constructor validation enhancement.The addition of
InvalidConfigvalidation for zero addresses aligns with the coding guidelines requirement to rigorously validate constructor inputs.
132-151: Robust validation logic for chain IDs and receiver addresses.The enhanced validation properly:
- Ensures destination chain ID consistency between bridge data and AllBridge data
- Handles EVM vs non-EVM receiver address validation correctly
- Uses appropriate custom errors for different failure scenarios
This aligns with the facet conventions for validating
targetChainIdagainstbridgeData.destinationChainand proper non-EVM address handling.
194-225: Efficient chain ID mapping implementation with appropriate error handling.The
_getAllBridgeChainIdfunction implements an efficient if-else chain for known mappings and properly reverts for unsupported chains. The logic correctly handles both direct mappings (where IDs are the same) and custom mappings.
164-164: No overflow risk when casting addresses
Ethereum addresses are 20 bytes (160 bits), so converting anaddresstouint160can never overflow. The patternbytes32(uint256(uint160(x)))is used consistently across the codebase (e.g., OFTComposeMsgCodec.sol, AllBridgeFacet.sol, RelayFacet.sol, GlacisFacet.sol, CelerCircleBridgeFacet.sol) to pack an address into a
bytes32. No additional bounds checks are necessary here.Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Facets/AllBridgeFacet.sol (2)
26-45: Consider consolidating chain ID constants with LiFiData.Since the contract now inherits from
LiFiDatawhich centralizes chain ID constants, consider whether these LiFi chain ID constants (lines 37-44) should be moved to theLiFiDatahelper to avoid duplication and maintain consistency across facets.
190-225: Well-implemented chain ID mapping function.The
_getAllBridgeChainIdfunction provides clean mapping logic with:
- Efficient search pattern splitting cases in half
- Clear handling of direct vs. custom chain ID mappings
- Proper error handling for unsupported chains
- Good documentation
However, there's a documentation error in the @return comment.
Fix the documentation:
-/// @return The corresponding Chainflip chain ID +/// @return The corresponding AllBridge chain ID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Facets/AllBridgeFacet.sol(6 hunks)src/Helpers/LiFiData.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Helpers/LiFiData.sol
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
src/Facets/*Facet.sol
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- conventions.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1169
File: script/deploy/zksync/DeployFeeCollector.zksync.s.sol:32-37
Timestamp: 2025-05-27T12:00:43.940Z
Learning: The lifinance/contracts repository has deployment scripts that perform validation checks (including zero-address validation) before executing individual deploy scripts, making runtime validation checks in the deploy scripts themselves redundant.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: mirooon
PR: lifinance/contracts#1187
File: .github/workflows/versionControlAndAuditCheck.yml:596-612
Timestamp: 2025-06-05T11:25:43.443Z
Learning: The user mirooon has an internal system for tracking tickets and technical debt. They acknowledged the security concern about disabled commit hash verification in the audit workflow and plan to implement conditional logic to skip verification only for revert PRs in a future update.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
Learnt from: 0xDEnYO
PR: lifinance/contracts#963
File: script/deploy/zksync/DeploySymbiosisFacet.zksync.s.sol:21-34
Timestamp: 2025-01-30T02:51:43.006Z
Learning: Parameter validation for deployment scripts in the LiFi contracts is handled by pre-deployment scripts, making additional validation within individual deployment scripts unnecessary.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: mirooon
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:63-64
Timestamp: 2025-02-26T13:25:23.615Z
Learning: In the LiFi contracts repository, RPC URL validation is handled in the `getViemChainForNetworkName` helper function, which throws a descriptive error if the RPC URL is missing. This ensures that by the time the chain object is used elsewhere in the codebase, a valid RPC URL is guaranteed to exist.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: deployments/_deployments_log_file.json:34037-34080
Timestamp: 2025-06-13T08:30:26.220Z
Learning: LiFiDiamond contains generic withdrawal logic, so individual facet contracts (e.g., PioneerFacet) do not need their own Ether-withdraw functions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: mirooon
PR: lifinance/contracts#945
File: script/demoScripts/demoGlacis.ts:180-189
Timestamp: 2025-01-30T10:38:18.041Z
Learning: The executeTransaction helper function in the LiFi codebase handles error cases and transaction logging comprehensively, including try/catch structures, transaction receipt validation, and detailed console logging for transaction status tracking.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: script/helperFunctions.sh:110-133
Timestamp: 2025-07-15T03:34:58.827Z
Learning: In script/helperFunctions.sh, 0xDEnYO prefers to keep the existing file locking implementation (using EXIT trap) unchanged even when potential improvements are suggested, prioritizing working code stability over refactoring when the current solution is functional.
src/Facets/AllBridgeFacet.sol (34)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must always include the following three functions: _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : If facetData contains a targetChainId, the facet must verify it against bridgeData.destinationChain to ensure these values match. This check applies only to EVM-to-EVM transactions.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts typically inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, Validatable, and may use ECDSA.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For transactions targeting non-EVM chains, receiverAddress must be declared as bytes (not bytes32), and the facet must ensure that the non-EVM address is not zero, reverting with InvalidNonEVMReceiver if so. When a non-EVM address is used, bridgeData.receiver must contain LibAsset.NON_EVM_ADDRESS.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Each facet must properly use the following modifiers: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps / doesContainSourceSwaps, doesNotContainDestinationCalls / doesContainDestinationCalls.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : Facets must emit LiFiTransferStarted at the start of a successful transaction, LiFiTransferCompleted after successful execution, and LiFiTransferRecovered if a transaction fails or is refunded.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: ezynda3
PR: lifinance/contracts#1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Learnt from: ezynda3
PR: lifinance/contracts#953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-04T09:00:06.744Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: script/deploy/facets/DeployLiFiDEXAggregator.s.sol:33-35
Timestamp: 2025-01-21T11:07:36.236Z
Learning: In the LiFiDEXAggregator contract deployment (DeployLiFiDEXAggregator.s.sol), the refundWallet address from global.json is used as the owner parameter in the contract's constructor, which is then passed to the WithdrawablePeriphery base contract.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For facets with a receiverAddress parameter, it should be the first parameter in the {facetName}Data struct. The receiverAddress must always be explicitly validated against bridgeData.receiver in the function logic.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/**/*.sol : Validate constructor inputs rigorously: if an invalid value (e.g., address(0) or zero value) is provided, revert with a custom error such as InvalidConfig. Ensure tests cover these conditions.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-06-30T10:00:42.770Z
Learning: Applies to src/Facets/*Facet.sol : For native fees, the facet must use the _depositAndSwap variant that reserves the required fee before execution.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:21-23
Timestamp: 2025-05-14T16:28:30.376Z
Learning: When shifting by 16 bits per chain ID in the GasZip contracts, the MAX_CHAINID_LENGTH_ALLOWED constant should be set to 16 (since a uint256 can hold at most 16 chain IDs when using 16 bits per chain). This constant should be consistent between related contracts like GasZipPeriphery and GasZipFacet.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:119-131
Timestamp: 2025-02-13T08:54:06.846Z
Learning: In Solidity, when mapping between fixed values known at compile time (like chain IDs), using if-else statements with constants is preferred over mappings since mappings cannot be declared as constant or immutable.
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
🪛 GitHub Actions: Security Alerts Review
src/Facets/AllBridgeFacet.sol
[error] 176-176: Olympix Integrated Security: Performing a narrowing downcast may result in silent overflow due to bit truncation.
[error] 164-164: Olympix Integrated Security: Performing a narrowing downcast may result in silent overflow due to bit truncation.
[error] 175-175: Olympix Integrated Security: Unchecked zero values passed as parameters may be in error or an indication of potentially insecure code design.
[error] 128-128: Olympix Integrated Security: Reentrant functions which emit events after making an external call may lead to out-of-order events.
🔇 Additional comments (3)
src/Facets/AllBridgeFacet.sol (3)
72-76: LGTM! Constructor validation properly implemented.The zero address validation for the AllBridge router follows the coding guidelines for rigorous constructor input validation and uses the appropriate custom error.
132-152: Excellent validation enhancements for destination chain and receiver address.The validation logic properly:
- Verifies destination chain ID consistency between bridgeData and allBridgeData
- Handles both EVM and non-EVM receiver address validation
- Uses appropriate custom errors for different validation failures
This aligns perfectly with the PR objective to add parameter validation.
187-188: Event emission follows proper pattern.The
LiFiTransferStartedevent is correctly emitted after successful bridge execution, following facet conventions. The pipeline warning about reentrancy and event ordering appears to be a false positive since the external call completes successfully before the event emission.
mirooon
reviewed
Jul 15, 2025
…ter-validation-to-allbridge-lf-14440
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ter-validation-to-allbridge-lf-14440
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
conventions.md (1)
205-211: Fix the fenced code block formatting issue.The static analysis tool flagged that the fenced code block should have a language specified and should use indented style instead of fenced style for consistency.
- ``` + ```solidity if ( bridgeData.receiver != NON_EVM_ADDRESS ) { revert InvalidCallData(); } - ``` + ```
mirooon
approved these changes
Jul 16, 2025
8 tasks
This was referenced Jul 16, 2025
This was referenced Jul 23, 2025
Closed
This was referenced Aug 14, 2025
8 tasks
This was referenced Oct 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-14440
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)