Lf 15521 enhance permit2 proxy [GenericErrors v1.0.3,IERC1271 v1.0.0,Permit2Proxy v1.1.0] #1391
Conversation
WalkthroughReplaces ERC20Permit usage with a new IERC20PermitExtended interface in Permit2Proxy, adds packed-signature overloads for EIP‑2612, implements EIP‑3009 transferWithAuthorization (bytes and v/r/s variants), and updates tests with EIP‑3009 coverage and renamed test functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (36)📓 Common learnings📚 Learning: 2025-04-21T03:15:12.236ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2024-11-25T09:05:43.045ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-09-09T10:39:26.383ZApplied to files:
📚 Learning: 2025-04-22T09:04:44.244ZApplied to files:
📚 Learning: 2024-10-24T06:17:25.211ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-07-04T08:59:08.108ZApplied to files:
📚 Learning: 2025-08-07T10:20:01.383ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2025-01-21T11:04:46.116ZApplied to files:
📚 Learning: 2025-01-21T11:07:36.236ZApplied to files:
📚 Learning: 2024-10-04T09:01:56.514ZApplied to files:
📚 Learning: 2025-12-17T10:28:34.379ZApplied to files:
📚 Learning: 2025-06-25T06:27:38.873ZApplied to files:
📚 Learning: 2025-07-15T05:05:28.134ZApplied to files:
📚 Learning: 2024-11-26T01:17:13.212ZApplied to files:
📚 Learning: 2025-02-11T10:33:52.791ZApplied to files:
📚 Learning: 2025-08-07T10:20:01.383ZApplied to files:
📚 Learning: 2025-02-11T10:35:03.536ZApplied to files:
📚 Learning: 2025-08-29T10:02:09.041ZApplied to files:
📚 Learning: 2025-12-17T10:28:13.904ZApplied to files:
📚 Learning: 2025-12-17T10:28:13.904ZApplied to files:
📚 Learning: 2025-08-26T12:11:21.073ZApplied to files:
📚 Learning: 2025-10-10T10:56:04.861ZApplied to files:
📚 Learning: 2025-09-25T07:47:30.735ZApplied to files:
📚 Learning: 2025-08-26T12:11:21.073ZApplied to files:
📚 Learning: 2025-02-13T08:57:00.095ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2025-08-01T22:11:04.478ZApplied to files:
📚 Learning: 2025-02-12T09:44:12.961ZApplied to files:
📚 Learning: 2025-12-17T10:27:21.083ZApplied to files:
📚 Learning: 2025-09-04T09:26:35.060ZApplied to files:
📚 Learning: 2025-12-17T10:28:13.904ZApplied to files:
🪛 Gitleaks (8.30.0)deployments/arbitrum.diamond.staging.json[high] 220-220: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ 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)
🔇 Additional comments (4)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Test Coverage ReportLine Coverage: 85.13% (3019 / 3546 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/Errors/GenericErrors.sol(2 hunks)src/Interfaces/IERC1271.sol(1 hunks)src/Periphery/Permit2Proxy.sol(5 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Errors/GenericErrors.solsrc/Interfaces/IERC1271.soltest/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
src/Errors/GenericErrors.sol
📄 CodeRabbit inference engine (conventions.md)
Define and maintain all generic, cross-contract errors exclusively in src/Errors/GenericErrors.sol and bump @Custom:version when adding new errors
Files:
src/Errors/GenericErrors.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Errors/GenericErrors.solsrc/Interfaces/IERC1271.solsrc/Periphery/Permit2Proxy.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Errors/GenericErrors.solsrc/Interfaces/IERC1271.soltest/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
src/Interfaces/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/**/*.sol: All interfaces must reside under src/Interfaces, be in separate files from implementations, and use an I prefix (e.g., ILiFi)
Every interface must include NatSpec in this order: @title, @notice, @author LI.FI (https://li.fi), @Custom:version X.Y.Z
Files:
src/Interfaces/IERC1271.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Periphery/Permit2Proxy.t.sol
🧠 Learnings (15)
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Errors/GenericErrors.sol : Define and maintain all generic, cross-contract errors exclusively in src/Errors/GenericErrors.sol and bump custom:version when adding new errors
Applied to files:
src/Errors/GenericErrors.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-07-15T05:05:28.134Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-02-11T10:33:52.791Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Periphery/Permit2Proxy.sol
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/Permit2Proxy.sol
[failure] 359-359:
The contract is vulnerable to signature replay attacks, potentially allowing malicious actors to reuse valid signatures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/signature-replay-attacks
⏰ 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)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (20)
src/Errors/GenericErrors.sol (2)
2-2: LGTM! Version bump follows coding guidelines.The version increment from 1.0.2 to 1.0.3 correctly follows the coding guidelines for maintaining GenericErrors.sol when adding new errors.
25-25: LGTM! Error placement and naming are appropriate.The new
InvalidSignature()error follows proper Solidity naming conventions (PascalCase) and is appropriately placed alphabetically in the error list. This supports EIP-1271 signature validation flows as referenced in the AI summary.src/Interfaces/IERC1271.sol (2)
1-8: LGTM! Header follows coding guidelines.The interface correctly follows the coding guidelines with SPDX license identifier, proper pragma statement, and NatSpec header in the required order (@title, @notice, @author, @Custom:version).
9-18: LGTM! Standard EIP-1271 implementation with proper documentation.The interface correctly implements the standard EIP-1271 method for signature validation in smart contracts. The function signature matches the standard exactly, and the NatSpec documentation properly documents the magic value 0x1626ba7e, which is the keccak256 hash of "isValidSignature(bytes32,bytes)".
src/Periphery/Permit2Proxy.sol (8)
10-11: LGTM! Proper imports for EIP-1271 support.The new imports for
IERC1271andInvalidSignatureare correctly added to support the EIP-1271 functionality. The imports follow the established pattern in the codebase.
16-17: LGTM! Updated documentation and version.The NatSpec comment correctly reflects the new EIP-1271 capability, and the version bump to 1.1.0 appropriately indicates the addition of new functionality.
42-45: LGTM! Standard EIP-1271 magic value constant.The constant correctly uses the standard EIP-1271 magic value 0x1626ba7e and follows proper naming conventions.
92-102: LGTM! Clean refactoring to use helper function.The refactoring to use
_handleEOAPermitimproves code reusability and maintains the same functionality while preparing for the bytes signature variant.
135-136: LGTM! Proper deadline validation.The deadline check at the beginning of the function correctly prevents expired signatures from being processed, improving security.
138-142: LGTM! Proper delegated EOA detection logic.The prefix detection logic correctly identifies delegated EOA signatures using the 0xef0100 prefix and validates the expected total length of 68 bytes (3-byte prefix + 65-byte signature).
144-163: LGTM! Appropriate routing logic for signature types.The routing logic correctly distinguishes between EOAs (including delegated EOAs) and smart contract wallets, directing each to the appropriate handling function.
297-357: LGTM! Robust EOA permit handling with error management.The
_handleEOAPermitfunction properly:
- Handles delegated signature unpacking
- Validates signature length (65 bytes)
- Extracts signature components using assembly
- Implements comprehensive error handling for permit failures
- Falls back to checking existing allowances when permit fails
The error handling covers various failure modes while still allowing transactions to proceed if adequate allowance exists.
test/solidity/Periphery/Permit2Proxy.t.sol (8)
12-12: LGTM! Proper error imports for testing.The import of
InvalidSignatureandTransferFromFailederrors correctly supports the expanded test scenarios for EIP-1271 functionality.
83-101: LGTM! Comprehensive EIP-1271 mock implementations.The mock contracts properly implement the EIP-1271 interface:
MockEIP1271Walletcorrectly returns the magic value for authorized hashesMaliciousEIP1271Walletappropriately returns wrong magic value for testing invalid scenarios- Both follow standard EIP-1271 patterns used in production wallets
1047-1088: LGTM! Comprehensive EOA bytes format test.The test properly validates the new bytes signature format for EOAs, including proper EIP-2612 digest generation and signature packing. The test flow correctly mirrors the expected usage pattern.
1090-1145: LGTM! Excellent delegated EOA test with realistic scenario.The test properly simulates the delegated EOA flow with the 0xef0100 prefix, correctly demonstrating how smart contracts can submit transactions with delegated EOA signatures. The comments clearly explain the use case.
1147-1178: LGTM! Thorough error condition testing.The tests properly validate:
- Invalid prefix length detection (67 instead of 68 bytes)
- Invalid signature length for EOAs (64 instead of 65 bytes)
- Both tests correctly expect the
InvalidSignatureerrorAlso applies to: 1180-1206
1208-1259: LGTM! Comprehensive smart wallet success test.The test properly validates the complete EIP-1271 flow:
- Correct message hash construction matching the implementation
- Proper wallet authorization setup
- Token funding and approval
- Successful execution verification
The test demonstrates the expected integration pattern for smart contract wallets.
1261-1290: LGTM! Comprehensive error scenario coverage.The tests thoroughly validate error conditions:
- Invalid signature (unauthorized hash)
- Wrong magic value from malicious wallet
- Missing token approval (expecting
TransferFromFailed)These tests provide excellent coverage of failure modes in EIP-1271 interactions.
Also applies to: 1292-1319, 1321-1360
1362-1391: LGTM! Proper deadline expiration test.The test correctly validates that expired deadlines are rejected with
InvalidSignatureerror, confirming the deadline validation logic works as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/Permit2Proxy.md (2)
20-41: Old prototype doc is stale and has a wrong param description + missing return.
- Fix param doc: diamondCalldata is calldata to execute (not “Address of the token to be bridged” on Line 31).
- Add the return type to the snippet to match the contract: returns (bytes memory).
Apply this diff to the snippet:
-/// @param diamondCalldata Address of the token to be bridged +/// @param diamondCalldata calldata to execute ) public payable + returns (bytes memory)
43-46: Clarify delegated-EOA format and scope.Document that delegated-EOA signatures are exactly 68 bytes (0xef0100 prefix + 65-byte ECDSA) and only supported on the EOA path. This avoids ambiguity with 1271 signatures.
src/Periphery/Permit2Proxy.sol (2)
119-127: Name is misleading now.This overload also handles 1271 signatures; consider renaming to callDiamondWithSignature(...) and keeping the legacy EIP2612-only overload for backward compatibility. Not blocking.
294-354: Gas/copying: slice delegated signature without byte-by-byte loop; normalize v.
- Replace the 65-iteration copy with an assembly slice.
- Normalize v to 27/28 for broader token compatibility.
Apply this diff:
) internal { - bytes memory actualSignature; - - // Handle delegated signature by stripping prefix - if (isDelegatedEOA) { - // Skip the 3-byte prefix and extract the 65-byte signature - actualSignature = new bytes(65); - for (uint256 i = 0; i < 65; i++) { - actualSignature[i] = signature[i + 3]; - } - } else { - actualSignature = signature; - } + bytes memory actualSignature = signature; + if (isDelegatedEOA) { + assembly { + // actualSignature := signature[3:68] + let len := 65 + actualSignature := mload(0x40) + // new free mem = ptr + 32(length) + 65(padded to 96) + mstore(0x40, add(actualSignature, 0x80)) + mstore(actualSignature, len) + // copy 65 bytes from signature+3 into actualSignature+32 + // calldataload/mload depending on location; here `signature` is in memory + // so copy from memory: use memcpy + // copy 64 bytes + mstore(add(actualSignature, 0x20), mload(add(signature, 0x23))) + mstore(add(actualSignature, 0x40), mload(add(signature, 0x43))) + mstore8(add(actualSignature, 0x60), byte(0, mload(add(signature, 0x63)))) + } + } @@ assembly { r := mload(add(actualSignature, 0x20)) s := mload(add(actualSignature, 0x40)) v := byte(0, mload(add(actualSignature, 0x60))) } + if (v < 27) v += 27;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/Permit2Proxy.md(3 hunks)src/Periphery/Permit2Proxy.sol(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Periphery/Permit2Proxy.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Periphery/Permit2Proxy.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Periphery/Permit2Proxy.sol
{README.md,Deploy.md,docs/**}
📄 CodeRabbit inference engine (conventions.md)
Keep primary documentation up to date: README.md (overview/setup), Deploy.md (deployment), and docs/ (technical docs)
Files:
docs/Permit2Proxy.md
🧠 Learnings (16)
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2024-10-14T08:23:38.076Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-12-02T06:33:33.309Z
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`.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-09-23T02:04:16.323Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-07-15T05:05:28.134Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-02-11T10:33:52.791Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Periphery/Permit2Proxy.soldocs/Permit2Proxy.md
📚 Learning: 2024-10-31T09:09:38.568Z
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.
Applied to files:
docs/Permit2Proxy.md
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/Permit2Proxy.sol
[notice] 139-139:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[failure] 356-356:
The contract is vulnerable to signature replay attacks, potentially allowing malicious actors to reuse valid signatures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/signature-replay-attacks
🔇 Additional comments (7)
docs/Permit2Proxy.md (1)
140-140: LGTM – helpful dev note.src/Periphery/Permit2Proxy.sol (6)
10-11: Good: IERC1271 + InvalidSignature imported.
16-17: NatSpec and version bump look correct.
42-45: Good: ERC1271 magic value constant added.
92-103: Reasonable: pack v/r/s then reuse the EOA path.Minor: you can pass the 65-byte signature directly without packing if you already had it, but current approach is fine.
119-174: Critical: 1271 path is replayable; add nonce + domain separation and consume it.Current 1271 hash lacks nonce/chain/contract binding. A valid signature can be replayed multiple times while deadline is valid and across chains/contracts. Gate with a per-wallet nonce and include chainId + contract in the hash; consume nonce on use.
Apply this diff to the public entry point (adds nonce and passes it through):
- /// @param signature Packed signature bytes (r,s,v for EOAs, arbitrary format for smart contracts) + /// @param signature Packed signature bytes (r,s,v for EOAs, arbitrary format for smart contracts) + /// @param nonce Replay‑protection nonce for 1271 wallets (ignored for EOA path) /// @param diamondCalldata calldata to execute - function callDiamondWithEIP2612Signature( + function callDiamondWithEIP2612Signature( address tokenAddress, uint256 amount, uint256 deadline, - bytes calldata signature, + bytes calldata signature, + uint256 nonce, bytes calldata diamondCalldata ) public payable returns (bytes memory) { @@ - if (msg.sender.code.length == 0 || isDelegatedEOA) { + if (msg.sender.code.length == 0 || isDelegatedEOA) { // EOA path - handle permit _handleEOAPermit( tokenAddress, amount, deadline, signature, isDelegatedEOA ); } else { // Smart contract wallet path - handle EIP-1271 - _handleEIP1271( + _handleEIP1271( tokenAddress, amount, deadline, - signature, + signature, + nonce, diamondCalldata ); }Add storage and expose current nonces (place in “Storage” section):
// 1271 per-wallet nonces for replay protection mapping(address => uint256) public eip1271Nonces;And update _handleEIP1271 as in my later comment.
356-384: Critical: 1271 message hash lacks nonce/chain/contract; consume nonce on success.Bind the signature to this contract and chain and make it one‑time via per‑wallet nonce consumption.
Apply this diff:
- ) internal view { - // Construct message hash for validation - bytes32 messageHash = keccak256( - abi.encode( - keccak256( - "PermitProxyCall(address token,uint256 amount,uint256 deadline,bytes32 calldataHash)" - ), - tokenAddress, - amount, - deadline, - keccak256(diamondCalldata) - ) - ); + uint256 nonce, + bytes calldata diamondCalldata + ) internal { + // Enforce expected nonce and build replay-protected domain-bound hash + uint256 expected = eip1271Nonces[msg.sender]; + if (nonce != expected) revert InvalidSignature(); + bytes32 messageHash = keccak256( + abi.encode( + keccak256( + "PermitProxyCall(address token,uint256 amount,uint256 deadline,bytes32 calldataHash,address verifyingContract,uint256 chainId,uint256 nonce)" + ), + tokenAddress, + amount, + deadline, + keccak256(diamondCalldata), + address(this), + block.chainid, + nonce + ) + ); @@ - if (magicValue != ERC1271_MAGIC_VALUE) { - revert InvalidSignature(); - } + if (magicValue != ERC1271_MAGIC_VALUE) revert InvalidSignature(); + // Consume nonce + unchecked { eip1271Nonces[msg.sender] = expected + 1; }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Periphery/Permit2Proxy.sol (2)
24-26: Good: per-wallet nonces for 1271 replay protection; consider naming clarity“nonces” can be confused with Permit2 bitmap nonces already exposed in this contract. Consider renaming to eip1271Nonces for clarity.
Apply this minimal diff:
- mapping(address => uint256) public nonces; + mapping(address => uint256) public eip1271Nonces;And in _handleEIP1271:
- uint256 currentNonce = nonces[msg.sender]; + uint256 currentNonce = eip1271Nonces[msg.sender]; ... - nonces[msg.sender] = currentNonce + 1; + eip1271Nonces[msg.sender] = currentNonce + 1;
281-327: Normalize ECDSA v for broader signer compatibilitySome signers/libraries emit v as 0/1. Normalize to 27/28 before calling permit to avoid brittle failures.
Apply:
assembly { r := mload(add(signature, 0x20)) s := mload(add(signature, 0x40)) v := byte(0, mload(add(signature, 0x60))) } + if (v < 27) { + v += 27; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/Permit2Proxy.md(3 hunks)src/Periphery/Permit2Proxy.sol(5 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/solidity/Periphery/Permit2Proxy.t.sol
- docs/Permit2Proxy.md
🧰 Additional context used
📓 Path-based instructions (3)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Periphery/Permit2Proxy.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Periphery/Permit2Proxy.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Periphery/Permit2Proxy.sol
🧠 Learnings (14)
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-12-02T06:33:33.309Z
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`.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-09-23T02:04:16.323Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-07-15T05:05:28.134Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-02-11T10:33:52.791Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Periphery/Permit2Proxy.sol
🔇 Additional comments (6)
src/Periphery/Permit2Proxy.sol (6)
10-11: LGTM: 1271 + error imports addedInterfaces and error type are appropriate for the new validation path.
16-18: Version/NatSpec updated — propagate docs and referencesPlease update any docs, deployment notes, and integrator references to 1.1.0 to avoid mismatches.
47-48: LGTM: ERC‑1271 magic value constantConstant is correct and scoped appropriately.
95-99: Packing v,r,s into bytes is fineNo issues; matches extraction logic below.
329-368: Replay protection for 1271: solid fix; includes nonce, chainId, and contract domainThis addresses prior “signature replay” findings by binding to nonce, chain, and verifying contract and consuming the nonce before external effects.
Please ensure tests cover:
- Cross‑chain replay attempts fail (different chainId).
- Cross‑contract replay attempts fail (different verifying contract).
- Same‑nonce double‑spend fails even with reentrancy via LIFI_DIAMOND.
You can grep tests quickly:
#!/bin/bash rg -nP -C2 'isValidSignature|ERC1271_MAGIC_VALUE|nonce[s]?\\[|chainid|contractAddress' --type=sol test
116-130: Rename overloaded function to callDiamondWithSignature, add NatSpec @return, use leading-underscore params, and keep a backward-compatible aliasFile: src/Periphery/Permit2Proxy.sol:116-130
- Rename the misleading overload to callDiamondWithSignature and update NatSpec: @param _tokenAddress, _param _amount, @param _deadline, @param _signature, @param _diamondCalldata; add @return data (bytes).
- Use leading-underscore param names throughout the body (check _deadline, call _handleEOAPermit/_handleEIP1271 with underscored args, transfer/approve using _tokenAddress/_amount, and call _executeCalldata(_diamondCalldata)).
- Keep a thin external wrapper named callDiamondWithEIP2612Signature(...) that delegates to callDiamondWithSignature for backward compatibility.
- Verify repository and integrator impact before merging (search callers, e.g., rg -nP '\bcallDiamondWithEIP2612Signature\s*(') to avoid breaking external integrations.
🤖 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: ✅ No unresolved security alerts! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/solidity/Periphery/Permit2Proxy.t.sol (1)
1118-1177: Avoid duplicating the EIP‑1271 type string in testsRecommend exposing PERMIT_PROXY_CALL_TYPEHASH (or a helper that returns the hash) from the proxy and referencing it here to eliminate drift risk if the type string changes.
Would you like me to add a view constant/getter in the proxy and adjust this test accordingly?
src/Periphery/Permit2Proxy.sol (3)
124-161: Naming/API clarity: EIP‑2612 function now accepts EIP‑1271 tooThe overloaded bytes version handles both EOAs and 1271 wallets but keeps the EIP‑2612 name. Consider adding an alias (e.g., callDiamondWithSignature) that forwards to this overload to make intent clear without breaking ABI.
367-369: Optional: emit an event when consuming 1271 noncesHelps off‑chain tracking and auditability.
Example:
event EIP1271NonceUsed(address indexed wallet, uint256 nonce);Emit before increment: emit EIP1271NonceUsed(msg.sender, currentNonce);
131-147: Design note: 1271 path restricted to wallet-calls onlyCurrent check requires msg.sender to be the wallet. If meta‑tx/relayer support is desired later, allow third parties to call with a wallet address parameter and verify via IERC1271(wallet).isValidSignature(...). Safe to defer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Periphery/Permit2Proxy.sol(5 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Periphery/Permit2Proxy.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Periphery/Permit2Proxy.t.sol
🧠 Learnings (17)
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-12-02T06:33:33.309Z
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`.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-09-23T02:04:16.323Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-07-15T05:05:28.134Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-02-11T10:33:52.791Z
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.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/Permit2Proxy.sol
[notice] 297-297:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[notice] 298-298:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[notice] 300-300:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
⏰ 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)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (7)
test/solidity/Periphery/Permit2Proxy.t.sol (7)
83-101: Mocks: EIP-1271 coverage looks solidThe mock/wallet contracts accurately model happy/wrong-magic paths and keep state minimal. No issues.
1047-1089: This test will fail until signature parsing for EOAs is fixedThe new EOA bytes path depends on correct (r,s,v) extraction in the proxy. Current implementation in _handleEOAPermit is broken; fix there and this should pass.
1090-1116: Good negative: invalid EOA signature lengthAsserts the custom InvalidSignature error; aligns with proxy checks.
1179-1208: Negative path: unauthorized EIP‑1271 signatureCovers invalid authorization correctly; assert on InvalidSignature matches proxy behavior.
1210-1237: Negative path: wrong magic valueGood coverage of wallets that return non‑standard magic; matches spec expectations.
1239-1285: Approval missing path is meaningfulAsserts TransferFromFailed, proving token approval is still required for 1271 flows.
1286-1315: Expired deadline checkCovers expiry in the new bytes‑based entry point; matches proxy’s InvalidSignature revert.
src/Periphery/Permit2Proxy.sol
Outdated
| /// Constants /// | ||
|
|
||
| bytes4 internal constant ERC1271_MAGIC_VALUE = 0x1626ba7e; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define and reuse a PERMIT_PROXY_CALL_TYPEHASH constant
Reduce risk of typos and enable reuse in tests by exposing a constant for the 1271 struct type hash.
Apply this diff:
/// Constants ///
-bytes4 internal constant ERC1271_MAGIC_VALUE = 0x1626ba7e;
+bytes4 internal constant ERC1271_MAGIC_VALUE = 0x1626ba7e;
+bytes32 public constant PERMIT_PROXY_CALL_TYPEHASH = keccak256(
+ "PermitProxyCall(address token,uint256 amount,uint256 deadline,bytes32 calldataHash,uint256 nonce,uint256 chainId,address contractAddress)"
+);And update usage:
- bytes32 messageHash = keccak256(
- abi.encode(
- keccak256(
- "PermitProxyCall(address token,uint256 amount,uint256 deadline,bytes32 calldataHash,uint256 nonce,uint256 chainId,address contractAddress)"
- ),
+ bytes32 messageHash = keccak256(
+ abi.encode(
+ PERMIT_PROXY_CALL_TYPEHASH,
tokenAddress,
amount,
deadline,
keccak256(diamondCalldata),
currentNonce,
block.chainid,
address(this)
)
);Also applies to: 340-354
🤖 Prompt for AI Agents
In src/Periphery/Permit2Proxy.sol around lines 45-48, add a new bytes32 constant
PERMIT_PROXY_CALL_TYPEHASH (computed as keccak256 of the 1271 struct definition)
alongside ERC1271_MAGIC_VALUE so tests and code can reuse it; then replace any
hard-coded/inline typehash computations or duplicated keccak256 calls with this
constant at the other occurrences (notably around lines 340-354) to avoid typos
and ensure a single source of truth for the 1271 permit proxy struct type hash.
src/Periphery/Permit2Proxy.sol
Outdated
| function _handleEOAPermit( | ||
| address tokenAddress, | ||
| uint256 amount, | ||
| uint256 deadline, | ||
| bytes memory signature | ||
| ) internal { | ||
| // Validate signature length | ||
| if (signature.length != 65) revert InvalidSignature(); | ||
|
|
||
| // Extract r, s, v from packed signature | ||
| bytes32 r; | ||
| bytes32 s; | ||
| uint8 v; | ||
|
|
||
| // Copy bytes manually without assembly | ||
| for (uint256 i = 0; i < 32; i++) { | ||
| r |= bytes32(signature[i]) >> (i * 8); | ||
| s |= bytes32(signature[i + 32]) >> (i * 8); | ||
| } | ||
| v = uint8(signature[64]); | ||
|
|
||
| // Call permit | ||
| try | ||
| ERC20Permit(tokenAddress).permit( | ||
| msg.sender, | ||
| address(this), | ||
| amount, | ||
| deadline, | ||
| v, | ||
| r, | ||
| s | ||
| ) | ||
| {} catch Error(string memory reason) { | ||
| if ( | ||
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | ||
| amount | ||
| ) { | ||
| revert(reason); | ||
| } | ||
| } catch (bytes memory reason) { | ||
| if ( | ||
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | ||
| amount | ||
| ) { | ||
| LibUtil.revertWith(reason); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocker: EOA signature parsing is incorrect (r/s assembly needed)
The loop that builds r and s zeroes out most bytes; permits will fail for valid signatures. Replace with standard 65‑byte unpacking and normalize v.
Apply this diff:
function _handleEOAPermit(
address tokenAddress,
uint256 amount,
uint256 deadline,
bytes memory signature
) internal {
- // Validate signature length
- if (signature.length != 65) revert InvalidSignature();
-
- // Extract r, s, v from packed signature
- bytes32 r;
- bytes32 s;
- uint8 v;
-
- // Copy bytes manually without assembly
- for (uint256 i = 0; i < 32; i++) {
- r |= bytes32(signature[i]) >> (i * 8);
- s |= bytes32(signature[i + 32]) >> (i * 8);
- }
- v = uint8(signature[64]);
+ if (signature.length != 65) revert InvalidSignature();
+ bytes32 r;
+ bytes32 s;
+ uint8 v;
+ // Unpack (r,s,v) from bytes[65]
+ assembly {
+ r := mload(add(signature, 0x20))
+ s := mload(add(signature, 0x40))
+ v := byte(0, mload(add(signature, 0x60)))
+ }
+ // Normalize v to {27,28}
+ if (v < 27) v += 27;
+ if (v != 27 && v != 28) revert InvalidSignature();
// Call permit
try
ERC20Permit(tokenAddress).permit(
msg.sender,
address(this),
amount,
deadline,
v,
r,
s
)
{} catch Error(string memory reason) {
if (
IERC20(tokenAddress).allowance(msg.sender, address(this)) <
amount
) {
revert(reason);
}
} catch (bytes memory reason) {
if (
IERC20(tokenAddress).allowance(msg.sender, address(this)) <
amount
) {
LibUtil.revertWith(reason);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _handleEOAPermit( | |
| address tokenAddress, | |
| uint256 amount, | |
| uint256 deadline, | |
| bytes memory signature | |
| ) internal { | |
| // Validate signature length | |
| if (signature.length != 65) revert InvalidSignature(); | |
| // Extract r, s, v from packed signature | |
| bytes32 r; | |
| bytes32 s; | |
| uint8 v; | |
| // Copy bytes manually without assembly | |
| for (uint256 i = 0; i < 32; i++) { | |
| r |= bytes32(signature[i]) >> (i * 8); | |
| s |= bytes32(signature[i + 32]) >> (i * 8); | |
| } | |
| v = uint8(signature[64]); | |
| // Call permit | |
| try | |
| ERC20Permit(tokenAddress).permit( | |
| msg.sender, | |
| address(this), | |
| amount, | |
| deadline, | |
| v, | |
| r, | |
| s | |
| ) | |
| {} catch Error(string memory reason) { | |
| if ( | |
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | |
| amount | |
| ) { | |
| revert(reason); | |
| } | |
| } catch (bytes memory reason) { | |
| if ( | |
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | |
| amount | |
| ) { | |
| LibUtil.revertWith(reason); | |
| } | |
| } | |
| } | |
| function _handleEOAPermit( | |
| address tokenAddress, | |
| uint256 amount, | |
| uint256 deadline, | |
| bytes memory signature | |
| ) internal { | |
| if (signature.length != 65) revert InvalidSignature(); | |
| bytes32 r; | |
| bytes32 s; | |
| uint8 v; | |
| // Unpack (r,s,v) from bytes[65] | |
| assembly { | |
| r := mload(add(signature, 0x20)) | |
| s := mload(add(signature, 0x40)) | |
| v := byte(0, mload(add(signature, 0x60))) | |
| } | |
| // Normalize v to {27,28} | |
| if (v < 27) v += 27; | |
| if (v != 27 && v != 28) revert InvalidSignature(); | |
| // Call permit | |
| try | |
| ERC20Permit(tokenAddress).permit( | |
| msg.sender, | |
| address(this), | |
| amount, | |
| deadline, | |
| v, | |
| r, | |
| s | |
| ) | |
| {} catch Error(string memory reason) { | |
| if ( | |
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | |
| amount | |
| ) { | |
| revert(reason); | |
| } | |
| } catch (bytes memory reason) { | |
| if ( | |
| IERC20(tokenAddress).allowance(msg.sender, address(this)) < | |
| amount | |
| ) { | |
| LibUtil.revertWith(reason); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 297-297:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[notice] 298-298:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[notice] 300-300:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
🤖 Prompt for AI Agents
In src/Periphery/Permit2Proxy.sol around lines 281 to 328, the manual loop that
tries to build r and s from the signature is incorrect and zeroes most bytes;
replace it with standard 65-byte unpacking using assembly (e.g., mload at
signature+32 for r and signature+64 for s) and set v = uint8(signature[64]) then
normalize v to 27 or 28 if it is 0 or 1 before calling permit; keep the existing
signature.length == 65 check and the existing try/catch behavior but use the
correctly-extracted r, s, v values.
There was a problem hiding this comment.
Are you certain it's not possible to split a signature without solidity using a modern solidity version?
There was a problem hiding this comment.
🧩 Analysis chain
@ezynda3 Let me check the current best practices by examining how signature splitting is handled in your codebase and checking for any existing patterns or OpenZeppelin usage.
🏁 Script executed:
#!/bin/bash
# Check if OpenZeppelin's ECDSA library is used in the codebase
rg -n "ECDSA" --type=sol -C3
# Look for signature splitting patterns in the codebase
rg -n "assembly.*signature" --type=sol -C5
# Check for any existing r, s, v extraction patterns
rg -nP "\b(bytes32\s+r|bytes32\s+s|uint8\s+v)" --type=sol -C3
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check if OpenZeppelin's ECDSA library is used in the codebase
rg -n "ECDSA" -g "*.sol" -C3
# Look for signature splitting patterns in the codebase
rg -n "assembly.*signature" -g "*.sol" -C5
# Check for any existing r, s, v extraction patterns
rg -nP "\b(bytes32\s+r|bytes32\s+s|uint8\s+v)" -g "*.sol" -C3
# Look for OpenZeppelin imports
rg -n "@openzeppelin" -g "*.sol"
# Check for existing signature handling methods
rg -n "signature.*split|split.*signature" -g "*.sol" -C3
Length of output: 42485
@ezynda3 You're absolutely right to question this! After examining your codebase, I can confirm that assembly is still the standard approach for splitting ECDSA signatures in modern Solidity, but you have better alternatives than writing custom assembly.
Key findings from your codebase:
-
Solady's ECDSA library is already used in
RelayFacet.sol:import { ECDSA } from "solady/utils/ECDSA.sol"; // Later used as: ECDSA.recover(message, signature)
-
Standard pattern everywhere else: Your test files consistently use:
(uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, msgHash); bytes memory signature = abi.encodePacked(r, s, v);
-
Even Safe contracts use assembly: The Safe contract has a
signatureSplitfunction that uses assembly for the same operation.
Why the manual loop fails:
Your current approach has this fundamental issue:
r |= bytes32(signature[i]) >> (i * 8); // This zeros most bytes instead of copying themRecommended solutions (in order of preference):
-
Use Solady's ECDSA library (already in your dependencies):
import { ECDSA } from "solady/utils/ECDSA.sol"; // Then just: ECDSA.recover(messageHash, signature)
-
Use the proven assembly pattern (like Safe contracts):
assembly { r := mload(add(signature, 0x20)) s := mload(add(signature, 0x40)) v := byte(0, mload(add(signature, 0x60))) }
-
Use OpenZeppelin's ECDSA library (also handles the assembly internally)
Since you're already using Solady in RelayFacet, would you prefer to refactor to use Solady's ECDSA.recover() instead of manually splitting signatures? This would eliminate the need for custom assembly while following patterns already established in your codebase.
🧠 Learnings used
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
ezynda3
force-pushed
the
LF-15521-enhance-permit2-proxy
branch
from
bd1fde6 to
0b118f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Periphery/Permit2Proxy.sol (1)
15-15: Update contract version to reflect new functionality.The contract has significant changes (new EIP-2612 overload with bytes signature and EIP-3009 support). The version should be incremented from
1.0.4to at least1.1.0.Apply this diff:
-/// @custom:version 1.0.4 +/// @custom:version 1.1.0Based on pipeline failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Interfaces/IERC20PermitExtended.sol(1 hunks)src/Periphery/Permit2Proxy.sol(4 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(21 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
src/Interfaces/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/**/*.sol: All interfaces must reside under src/Interfaces, be in separate files from implementations, and use an I prefix (e.g., ILiFi)
Every interface must include NatSpec in this order: @title, @notice, @author LI.FI (https://li.fi), @Custom:version X.Y.Z
Files:
src/Interfaces/IERC20PermitExtended.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Periphery/Permit2Proxy.t.sol
🧠 Learnings (12)
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
src/Interfaces/IERC20PermitExtended.solsrc/Periphery/Permit2Proxy.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Periphery/Permit2Proxy.soltest/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-07-15T05:05:28.134Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
src/Periphery/Permit2Proxy.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to test/solidity/**/*.t.sol : Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2024-10-10T03:26:23.793Z
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.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
test/solidity/Periphery/Permit2Proxy.t.sol
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Interfaces/IERC20PermitExtended.sol
[error] 1-1: The following files are missing a custom:version tag in their code: src/Interfaces/IERC20PermitExtended.sol
src/Periphery/Permit2Proxy.sol
[error] 1-1: The file has relevant changes but the version (v1.0.4) was not updated. This Github action cannot complete until these issues are solved.
⏰ 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)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (15)
src/Interfaces/IERC20PermitExtended.sol (2)
7-13: LGTM: Packed-signature permit overload.The permit function accepting
bytes calldata signatureprovides a convenient alternative to the standard (v, r, s) overload, allowing callers to pass packed signatures directly.
15-35: LGTM: EIP-3009 transferWithAuthorization overloads.Both overloads correctly implement the EIP-3009
transferWithAuthorizationfunction signature, providing both (v, r, s) and packed-signature variants for flexibility.src/Periphery/Permit2Proxy.sol (4)
8-8: LGTM: Updated import for extended interface.The import of
IERC20PermitExtendedcorrectly replaces the previousERC20Permitimport, enabling support for packed signatures and EIP-3009 flows.
64-116: LGTM: Bytes-signature EIP-2612 overload.The new overload correctly implements the same logic as the existing (v, r, s) variant, calling
IERC20PermitExtended.permitwith a packed signature. Error handling, token transfer, approval, and calldata execution are consistent with the existing implementation.
131-176: LGTM: Updated to use IERC20PermitExtended interface.The cast to
IERC20PermitExtendedcorrectly aligns with the new interface, maintaining the same permit logic with (v, r, s) parameters.
178-250: LGTM: EIP-3009 transferWithAuthorization flows.Both overloads correctly implement EIP-3009
transferWithAuthorization, which atomically transfers tokens from the user to the proxy (unlikepermit, which only grants approval). The absence of try-catch error handling is appropriate here, as there's no allowance fallback scenario. The subsequentmaxApproveto the diamond and calldata execution are consistent with the EIP-2612 flows.test/solidity/Periphery/Permit2Proxy.t.sol (9)
113-123: LGTM: TestDataEIP3009 struct.The struct correctly captures EIP-3009 parameters, including
bytes32 nonce(as required by EIP-3009, unlike EIP-2612'suint256 nonce).
133-133: LGTM: Fork block updated.The fork block number update ensures tests run against the latest on-chain state, which is necessary for testing EIP-2612 and EIP-3009 flows with real contracts like USDC.
162-200: LGTM: Test renamed to CamelCase convention.The test name follows the
test_prefix convention and uses CamelCase for readability, consistent with modern Solidity test naming practices.
202-238: LGTM: Bytes-signature EIP-2612 test.The new test correctly verifies the bytes-signature overload by packing (r, s, v) into
bytes memory signatureand callingcallDiamondWithEIP2612Signaturewith the packed signature.
240-383: LGTM: Test renames to CamelCase convention.All test renames follow the
test_ortestRevert_prefix convention with CamelCase, improving readability and consistency.
387-424: LGTM: EIP-3009 signature test with (v, r, s).The test correctly verifies the EIP-3009 flow using
transferWithAuthorizationwith separated signature components (v, r, s), ensuring tokens are transferred and the diamond call succeeds.
426-465: LGTM: EIP-3009 signature test with bytes.The test correctly verifies the EIP-3009 flow using a packed signature, covering both overloads of
callDiamondWithEIP3009Signature.
467-907: LGTM: Test renames and existing test coverage.All test renames follow the established convention. The existing test coverage for Permit2 flows, error handling, and edge cases remains comprehensive.
1142-1199: LGTM: EIP-3009 test helper functions.Both helper functions correctly implement EIP-3009 signature generation:
_getTestDataEIP3009SignedBypermit2Usergenerates test data with a unique nonce (hashed from user and timestamp) and signs the EIP-3009 digest._generateEIP3009MsgHashconstructs the EIP-712 digest matching the EIP-3009TransferWithAuthorizationtype hash.
| @@ -0,0 +1,36 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Fix the license identifier to LGPL-3.0-only.
The coding guidelines require all first-party Solidity files to use LGPL-3.0-only, not MIT.
Apply this diff:
-// SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: LGPL-3.0-onlyBased on coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SPDX-License-Identifier: MIT | |
| // SPDX-License-Identifier: LGPL-3.0-only |
🧰 Tools
🪛 GitHub Actions: VersionControlAndAuditVerification
[error] 1-1: The following files are missing a custom:version tag in their code: src/Interfaces/IERC20PermitExtended.sol
🤖 Prompt for AI Agents
In src/Interfaces/IERC20PermitExtended.sol around line 1 the SPDX license
identifier is MIT but project guidelines require LGPL-3.0-only; update the
file's first line to use "SPDX-License-Identifier: LGPL-3.0-only" (replace the
existing identifier exactly) and save the file.
|
|
||
| import { IERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol"; | ||
|
|
||
| interface IERC20PermitExtended is IERC20Permit { |
There was a problem hiding this comment.
Add required NatSpec documentation for the interface.
All interfaces must include NatSpec with @title, @notice, @author LI.FI (https://li.fi), and @custom:version X.Y.Z (in that order) before the interface declaration.
Apply this diff:
import { IERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol";
+/// @title IERC20 Permit Extended Interface
+/// @notice Extends IERC20Permit with packed signature support and EIP-3009 transferWithAuthorization
+/// @author LI.FI (https://li.fi)
+/// @custom:version 1.0.0
interface IERC20PermitExtended is IERC20Permit {Based on coding guidelines and pipeline failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface IERC20PermitExtended is IERC20Permit { | |
| import { IERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol"; | |
| /// @title IERC20 Permit Extended Interface | |
| /// @notice Extends IERC20Permit with packed signature support and EIP-3009 transferWithAuthorization | |
| /// @author LI.FI (https://li.fi) | |
| /// @custom:version 1.0.0 | |
| interface IERC20PermitExtended is IERC20Permit { |
🤖 Prompt for AI Agents
In src/Interfaces/IERC20PermitExtended.sol around line 6, the interface
declaration lacks the required NatSpec block; add a NatSpec comment immediately
above the `interface IERC20PermitExtended is IERC20Permit {` line containing
@title, @notice, @author LI.FI (https://li.fi), and @custom:version X.Y.Z in
that exact order, providing brief descriptive text for @title and @notice and
keeping the author and version tags as specified.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)