Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit 2 trusted forwarder lf 11889 #939

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

0xDEnYO
Copy link
Contributor

@0xDEnYO 0xDEnYO commented Jan 22, 2025

Which Jira task belongs to this PR?

https://lifi.atlassian.net/browse/LF-11889

Why did I implement it this way?

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

Copy link
Contributor

coderabbitai bot commented Jan 22, 2025

Walkthrough

This pull request introduces significant changes to the Permit2Proxy contract and related configurations across multiple blockchain networks. The primary focus is on implementing support for trusted forwarders using the ERC2771 standard, which enables gasless transactions. The changes involve updating configuration files, deployment scripts, and adding a new ERC2771ContextCustom contract to manage trusted forwarders. The modifications enhance the contract's flexibility and transaction handling capabilities while maintaining existing functionality.

Changes

File Change Summary
config/permit2Proxy.json Restructured JSON configuration with nested objects for Permit2 and TrustedForwarders
deployments/_deployments_log_file.json Added new entries for version "1.1.0" in production and staging environments
deployments/arbitrum.* Updated Permit2Proxy address, removed MayanFacet
deployments/optimism.* Added Permit2Proxy address with new configuration
script/deploy/_targetState.json Added new "staging" environment configuration for LiFiDiamond
src/Periphery/Permit2Proxy.sol Updated contract to support trusted forwarders, added new constructor and method
src/Security/ERC2771ContextCustom.sol New contract for managing trusted forwarders
test/solidity/Periphery/Permit2Proxy.t.sol Added tests for trusted forwarder functionality

Possibly related PRs

Suggested labels

AuditRequired, AuditCompleted

Suggested reviewers

  • maxklenk

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (6)
src/Security/ERC2771ContextCustom.sol (1)

57-67: Emit events after state changes

In the setTrustedForwarders function, consider emitting the TrustedForwardersUpdated event after updating the _trustedForwarders mapping to ensure the event reflects the updated state.

Apply this diff:

             if (_forwarderAddresses.length != _isTrusted.length)
                 revert InvalidCallData();

-            emit TrustedForwardersUpdated(_forwarderAddresses, _isTrusted);

             // update forwarder addresses
             for (uint i; i < _forwarderAddresses.length; ) {
                 _trustedForwarders[_forwarderAddresses[i]] = _isTrusted[i];

                 unchecked {
                     ++i;
                 }
             }

+            emit TrustedForwardersUpdated(_forwarderAddresses, _isTrusted);
🧰 Tools
🪛 GitHub Actions: Version Control Checker

[error] Missing custom:version tag in the contract code. Every version-controlled contract needs to have a custom:version tag.

src/Periphery/Permit2Proxy.sol (2)

11-11: Standardize error naming to follow conventions

Consider renaming the UnAuthorized error to Unauthorized to match common Solidity naming conventions and improve consistency.

Apply this diff:

 import { LibAsset, IERC20 } from "lifi/Libraries/LibAsset.sol";
 import { PermitHash } from "permit2/libraries/PermitHash.sol";
 import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";
 import { WithdrawablePeriphery } from "lifi/Helpers/WithdrawablePeriphery.sol";
-import { ERC2771ContextCustom } from "lifi/Security/ERC2771ContextCustom.sol";
-import { UnAuthorized } from "../Errors/GenericErrors.sol";
+import { ERC2771ContextCustom } from "lifi/Security/ERC2771ContextCustom.sol";
+import { Unauthorized } from "../Errors/GenericErrors.sol";
 import { SafeTransferLib } from "solady/utils/SafeTransferLib.sol";
 import { console2 } from "forge-std/console2.sol";

 ...

             // only allow this function to be called by pre-whitelisted trusted forwarders
-            if (!isTrustedForwarder(msg.sender)) revert UnAuthorized();
+            if (!isTrustedForwarder(msg.sender)) revert Unauthorized();

Also applies to: 90-90

🧰 Tools
🪛 GitHub Actions: Version Control Checker

[info] Version successfully updated to 1.1.0

🪛 GitHub Actions: Audit Verifier

[error] Missing audit log entry for contract Permit2Proxy version 1.1.0. An audit must be completed and logged before the check can pass.


80-88: Consider declaring the function external instead of public

For consistency and potential gas savings, consider changing callDiamondWithEIP2612SignatureViaTrustedForwarder from public to external, as it does not call itself internally.

Apply this diff:

     /// @param diamondCalldata calldata to execute
-    function callDiamondWithEIP2612SignatureViaTrustedForwarder(
+    function callDiamondWithEIP2612SignatureViaTrustedForwarder(
+        address tokenAddress,
+        uint256 amount,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s,
+        bytes calldata diamondCalldata
+    ) external payable returns (bytes memory) {
🧰 Tools
🪛 GitHub Actions: Version Control Checker

[info] Version successfully updated to 1.1.0

🪛 GitHub Actions: Audit Verifier

[error] Missing audit log entry for contract Permit2Proxy version 1.1.0. An audit must be completed and logged before the check can pass.

test/solidity/Periphery/Permit2Proxy.t.sol (2)

136-141: Remove redundant if (!success) revert(); after vm.expectRevert

In the tests where vm.expectRevert is used, the subsequent check if (!success) revert(); is unnecessary as the test will automatically fail if the expected revert does not occur. Consider removing these lines to simplify the code.

Apply this diff to remove the redundant code:

             // expect tx to revert as we call fom untrusted address
             vm.expectRevert(UnAuthorized.selector);

             // call Permit2Proxy from Gelato contract with msgSender appended to calldata
             (bool success, ) = address(permit2Proxy).call(
                 callDataWithMsgSenderAppended
             );

-            if (!success) revert();

             vm.stopPrank();

Also applies to: 257-260


263-286: Add assertion messages to improve test clarity

In your assertions, consider adding descriptive messages to help identify the cause if a test fails. This aids in debugging and provides clearer feedback.

Example:

             // make sure address is not trusted yet
             assertEq(
                 ERC2771ContextCustom(address(permit2Proxy)).isTrustedForwarder(
                     USER_REFUND
                 ),
                 false
+                , "Address should not be trusted before the update"
             );
             // prepare arguments for trusted forwarder update
             address[] memory trustedForwarders = new address[](1);
             trustedForwarders[0] = USER_REFUND;

             bool[] memory isTrusted = new bool[](1);
             isTrusted[0] = true;

             // update trusted forwarder addresses
             permit2Proxy.setTrustedForwarders(trustedForwarders, isTrusted);

             // make sure update was successful
             assertEq(
                 ERC2771ContextCustom(address(permit2Proxy)).isTrustedForwarder(
                     USER_REFUND
                 ),
                 true
+                , "Trusted forwarder address was not updated correctly"
             );
config/permit2Proxy.json (1)

23-24: Check case sensitivity in blast network configuration.

The Permit2 address for blast network uses lowercase, while other networks use uppercase. Consider maintaining consistency.

-    "Permit2": "0x000000000022d473030f116ddee9f6b43ac78ba3",
+    "Permit2": "0x000000000022D473030F116dDEE9F6B43aC78BA3",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c49bae and f41cf66.

📒 Files selected for processing (12)
  • config/permit2Proxy.json (1 hunks)
  • deployments/_deployments_log_file.json (2 hunks)
  • deployments/arbitrum.diamond.staging.json (1 hunks)
  • deployments/arbitrum.staging.json (1 hunks)
  • deployments/optimism.diamond.staging.json (1 hunks)
  • deployments/optimism.staging.json (1 hunks)
  • package.json (2 hunks)
  • script/deploy/_targetState.json (2 hunks)
  • script/deploy/facets/DeployPermit2Proxy.s.sol (2 hunks)
  • src/Periphery/Permit2Proxy.sol (3 hunks)
  • src/Security/ERC2771ContextCustom.sol (1 hunks)
  • test/solidity/Periphery/Permit2Proxy.t.sol (4 hunks)
✅ Files skipped from review due to trivial changes (2)
  • package.json
  • deployments/arbitrum.diamond.staging.json
🧰 Additional context used
📓 Learnings (4)
deployments/optimism.diamond.staging.json (3)
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/aurora.diamond.json:75-75
Timestamp: 2024-11-26T01:16:48.020Z
Learning: On networks where the `Permit2Proxy` contract is not deployed, the `Permit2Proxy` address is intentionally left empty in the configuration files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/polygonzkevm.diamond.json:91-91
Timestamp: 2024-11-26T01:04:44.582Z
Learning: Ensure that the `Permit2Proxy` address is set to `0x6307119078556Fc8aD77781DFC67df20d75FB4f9` in the deployment files for all networks, including `scroll` and `fantom`.
script/deploy/facets/DeployPermit2Proxy.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/facets/DeployPermit2Proxy.s.sol:17-20
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In `DeployScriptBase`, `getConstructorArgs()` is called and constructor arguments are handled, so child deploy scripts do not need to pass constructor arguments explicitly to the `deploy` function.
src/Periphery/Permit2Proxy.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-11-12T09:43:10.543Z
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`).
script/deploy/_targetState.json (1)
Learnt from: ezynda3
PR: lifinance/contracts#807
File: script/deploy/_targetState.json:164-164
Timestamp: 2024-12-03T11:01:57.084Z
Learning: Version differences in `CalldataVerificationFacet` between staging and production are acceptable and not an issue.
🪛 GitHub Actions: Version Control Checker
src/Periphery/Permit2Proxy.sol

[info] Version successfully updated to 1.1.0

src/Security/ERC2771ContextCustom.sol

[error] Missing custom:version tag in the contract code. Every version-controlled contract needs to have a custom:version tag.

🪛 GitHub Actions: Audit Verifier
src/Periphery/Permit2Proxy.sol

[error] Missing audit log entry for contract Permit2Proxy version 1.1.0. An audit must be completed and logged before the check can pass.

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: enforce-min-test-coverage
  • GitHub Check: run-unit-tests
🔇 Additional comments (17)
src/Security/ERC2771ContextCustom.sol (1)

84-122: Ensure correct implementation of _msgSender and _msgData overrides

Verify that the _msgSender and _msgData functions correctly handle the extraction of the original sender and data when called through a trusted forwarder, in compliance with the ERC-2771 specification.

🧰 Tools
🪛 GitHub Actions: Version Control Checker

[error] Missing custom:version tag in the contract code. Every version-controlled contract needs to have a custom:version tag.

src/Periphery/Permit2Proxy.sol (2)

88-128: Ensure correct usage of _msgSender() with trusted forwarders

In the callDiamondWithEIP2612SignatureViaTrustedForwarder function, verify that _msgSender() correctly retrieves the original sender when called via a trusted forwarder, and that token transfers and approvals are secure and accurate.

🧰 Tools
🪛 GitHub Actions: Version Control Checker

[info] Version successfully updated to 1.1.0

🪛 GitHub Actions: Audit Verifier

[error] Missing audit log entry for contract Permit2Proxy version 1.1.0. An audit must be completed and logged before the check can pass.


70-128: Verify ERC2771 compliance in handling trusted forwarders

Confirm that the implementation correctly adheres to the ERC2771 standard, particularly in the way msg.sender is appended to msg.data and how _msgSender() and _msgData() are used. Ensure the contract securely handles meta-transactions through trusted forwarders.

🧰 Tools
🪛 GitHub Actions: Version Control Checker

[info] Version successfully updated to 1.1.0

🪛 GitHub Actions: Audit Verifier

[error] Missing audit log entry for contract Permit2Proxy version 1.1.0. An audit must be completed and logged before the check can pass.

test/solidity/Periphery/Permit2Proxy.t.sol (1)

223-229: Verify mocked call behavior in tests

Ensure that the use of vm.mockCallRevert accurately simulates the intended failure scenarios and that the tests correctly validate the contract's behavior when permit calls to the token fail.

Also applies to: 525-529

script/deploy/facets/DeployPermit2Proxy.s.sol (2)

48-53: LGTM! Proper handling of trusted forwarders configuration.

The implementation correctly retrieves and parses the trusted forwarders array from the configuration file.


68-74: Verify constructor arguments order.

The constructor arguments are correctly encoded, but let's verify the order matches the contract's constructor.

Run this script to check the constructor signature:

✅ Verification successful

Constructor arguments are correctly ordered

The encoding order matches exactly with the Permit2Proxy constructor parameters:

  • diamond_lifiDiamond
  • permit2Address_permit2
  • safeAddress_owner
  • trustedForwarders_trustedForwarderAddresses
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify constructor signature matches arguments order
# Expected: constructor(address diamond, address permit2, address safe, address[] memory trustedForwarders)

ast-grep --pattern 'contract Permit2Proxy {
  constructor($$$) {
    $$$
  }
}'

Length of output: 81


Script:

#!/bin/bash
# First, let's find the Permit2Proxy contract file
rg -l "contract Permit2Proxy"

# Then get the content around the constructor
rg -B2 -A5 "constructor.*Permit2Proxy" 

Length of output: 1277


Script:

#!/bin/bash
# Get the constructor definition from the main contract
rg -B2 -A6 "constructor.*\(" src/Periphery/Permit2Proxy.sol

Length of output: 290

deployments/optimism.staging.json (1)

43-44: LGTM! Contract addresses added correctly.

The RelayFacet and Permit2Proxy addresses are properly added to the deployment configuration.

deployments/arbitrum.staging.json (1)

49-49: Verify the removal of MayanFacet.

The Permit2Proxy address is updated correctly, but let's verify if MayanFacet removal is intentional.

Run this script to check for MayanFacet references:

config/permit2Proxy.json (3)

2-9: LGTM! Consistent configuration for mainnet and arbitrum.

The Permit2 addresses and TrustedForwarders are consistently configured for both networks.


34-37: Verify the security implications of mixed configurations.

Some networks have TrustedForwarders configured but empty Permit2 addresses. This unusual configuration needs verification.

Consider documenting the rationale for these mixed configurations and their implications for contract functionality.

Also applies to: 50-53, 62-65, 70-73, 74-77, 94-97, 98-101


10-13: Verify the implications of empty configurations.

Several networks have empty values for both Permit2 and TrustedForwarders. This might affect contract functionality on these networks.

Run this script to check deployment status on these networks:

Also applies to: 27-29, 39-41, 43-45, 47-49, 55-57, 59-61, 67-69, 79-81, 83-85, 103-105, 107-109, 115-117

✅ Verification successful

Empty configurations in permit2Proxy.json are intentional and do not affect functionality

The empty values for Permit2 and TrustedForwarders in these networks are intentional design choices, as these features are not required or used on these networks. This is evidenced by:

  • Active deployments and integrations exist for all these networks
  • Network-specific configurations are properly maintained in other config files
  • The networks are fully functional with various bridges and protocols
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check deployment status on networks with empty configurations
# Expected: No active deployments on these networks

# Get all deployment files
fd -e json -e staging -e production deployments/

Length of output: 404


Script:

#!/bin/bash
# First, let's find all JSON files that might contain deployment info
echo "=== Searching for deployment and configuration files ==="
fd -e json

echo -e "\n=== Checking for network references in code ==="
# Search for network names in the codebase
rg -i "aurora|optimism|arbitrum|polygon|base|celo" --type ts --type js --type json

echo -e "\n=== Checking documentation ==="
# Look for any documentation about network support
fd README -t f -d 2

Length of output: 53826

deployments/optimism.diamond.staging.json (1)

148-148: Verify the Permit2Proxy address for Optimism network.

The Permit2Proxy address being set (0xb33Fe241BEd9bf5F694101D7498F63a0d060F999) differs from the address mentioned in previous deployments (0x6307119078556Fc8aD77781DFC67df20d75FB4f9). Please confirm if this is intentionally different for the Optimism network.

Run the following script to check the contract deployment and verify its compatibility:

✅ Verification successful

The Permit2Proxy address is correct for the staging environment

The address 0xb33Fe241BEd9bf5F694101D7498F63a0d060F999 is consistently used across all staging environments, while production environments use a different address. This is an intentional configuration difference between staging and production deployments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the Permit2Proxy contract deployment and its compatibility

# Check if the contract exists and verify its code
echo "Checking contract existence and code at 0xb33Fe241BEd9bf5F694101D7498F63a0d060F999"
curl -s -X POST -H "Content-Type: application/json" --data '{
    "jsonrpc":"2.0",
    "method":"eth_getCode",
    "params":["0xb33Fe241BEd9bf5F694101D7498F63a0d060F999", "latest"],
    "id":1
}' https://optimism-mainnet.infura.io/v3/

# Search for any references to this address in the codebase
echo "Searching for references to this address in the codebase"
rg -i "0xb33Fe241BEd9bf5F694101D7498F63a0d060F999"

Length of output: 1247


Script:

#!/bin/bash
# Search for production deployment files and other Permit2Proxy addresses
echo "Searching for production deployment files:"
fd -e json . deployments/ | grep -v "staging" | grep -v "_log"

echo -e "\nSearching for all Permit2Proxy address configurations:"
rg -B1 -A1 '"Permit2Proxy":'

Length of output: 20713

script/deploy/_targetState.json (3)

397-446: LGTM! Staging environment for Arbitrum is properly configured.

The staging environment for Arbitrum mirrors the production configuration with appropriate facet versions.


497-544: LGTM! Staging environment for Optimism is properly configured.

The staging environment for Optimism mirrors the production configuration with appropriate facet versions.


421-421: Verify Permit2Proxy version compatibility.

The Permit2Proxy version is set to "1.0.0" in both staging environments. Since this PR implements trusted forwarders functionality, verify if this version supports the new features.

Run the following script to check version compatibility:

Also applies to: 521-521

✅ Verification successful

Version 1.0.0 is compatible with trusted forwarders functionality

The Permit2Proxy contract has built-in support for trusted forwarders through its ERC2771ContextCustom implementation, including configuration and management of trusted forwarder addresses. The functionality is already deployed and configured across multiple networks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check Permit2Proxy version compatibility with trusted forwarders

# Search for version declarations in Permit2Proxy contract
echo "Searching for version declarations in Permit2Proxy"
ast-grep --pattern 'contract Permit2Proxy {
  $$$
  string constant version = $VERSION;
  $$$
}'

# Search for trusted forwarder implementations
echo "Searching for trusted forwarder implementations"
ast-grep --pattern 'contract Permit2Proxy {
  $$$
  function isTrustedForwarder($ADDR) $$$
  $$$
}'

Length of output: 416


Script:

#!/bin/bash
# Find Permit2Proxy contract file
echo "Searching for Permit2Proxy contract file:"
fd -e sol -e js "Permit2Proxy"

# Search for trusted forwarder related code
echo -e "\nSearching for trusted forwarder references:"
rg -i "trustedforward|permit2proxy.*version" -A 2

Length of output: 13098

deployments/_deployments_log_file.json (2)

25851-25852: Verify deployment sequence and constructor arguments.

Issues found:

  1. Production deployment (07:56:36) preceded staging deployment (07:57:22). Typically, staging should be deployed first for testing.
  2. Constructor arguments differ between environments:
    • Production: ...9e606d0d2bba344b911e2f4eab95d9235a83fe15...
    • Staging: ...a8892ea3fddef2aa8afb1e3643a3284f978a5114...

Let's verify the constructor arguments:

Also applies to: 25915-25916


25849-25854: Verify trusted forwarder configuration.

Both environments use the same trusted forwarder address 0xd8253782c45a12053594b9deb72d8e8ab2fca54c. Let's verify this configuration:

Also applies to: 25913-25918

✅ Verification successful

Trusted forwarder configuration verified successfully

The address 0xd8253782c45a12053594b9deb72d8e8ab2fca54c is correctly configured as the Gelato 1Balance ERC2771 forwarder across environments with proper implementation and test coverage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify trusted forwarder configuration

# Search for trusted forwarder address in the codebase
echo "Searching for trusted forwarder address:"
rg -i "d8253782c45a12053594b9deb72d8e8ab2fca54c"

# Search for any ERC2771 related configurations
echo "Searching for ERC2771 configurations:"
rg -i "ERC2771|trustedForwarder"

Length of output: 10284

Comment on lines +31 to +34
/// We need to have a constructor to silence the compiler
/// However, we do not call the constructor of TransferrableOwnership here since the
/// contract is already initialized by WithdrawablePeriphery.sol and we cannot initialize twice
constructor(address[] memory _trustedForwarderAddresses) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update constructor comments to reflect accurate inheritance

The comment mentions WithdrawablePeriphery.sol, but this contract does not inherit from it. Verify the inheritance and update the comments to reflect the correct contracts involved.

Apply this diff to correct the comment:

     /// @notice Constructor
-    ///         We need to have a constructor to silence the compiler
-    ///         However, we do not call the constructor of TransferrableOwnership here since the
-    ///         contract is already initialized by WithdrawablePeriphery.sol and we cannot initialize twice
+    ///         Initializes the contract with the provided trusted forwarder addresses.

Alternatively, if the contract should inherit from WithdrawablePeriphery, adjust the inheritance accordingly.

📝 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.

Suggested change
/// We need to have a constructor to silence the compiler
/// However, we do not call the constructor of TransferrableOwnership here since the
/// contract is already initialized by WithdrawablePeriphery.sol and we cannot initialize twice
constructor(address[] memory _trustedForwarderAddresses) {
/// @notice Constructor
/// Initializes the contract with the provided trusted forwarder addresses.
constructor(address[] memory _trustedForwarderAddresses) {
🧰 Tools
🪛 GitHub Actions: Version Control Checker

[error] Missing custom:version tag in the contract code. Every version-controlled contract needs to have a custom:version tag.

Comment on lines +10 to +23
/**
* @dev Context variant with ERC-2771 support.
*
* WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll
* be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC-2771
* specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
* behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
* function only accessible if `msg.data.length == 0`.
*
* WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
* Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
* recovery.
*/
abstract contract ERC2771ContextCustom is Context, TransferrableOwnership {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add @custom:version tag to the contract documentation

The pipeline reports that the custom:version tag is missing. Please add an @custom:version tag in the contract's NatSpec comments to specify the contract version, ensuring compliance with version control requirements.

Apply this diff to add the version tag:

 /**
+ * @custom:version 1.0.0
  * @dev Context variant with ERC-2771 support.
  *
  * WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll
  * be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC-2771
  * specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
  * behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
  * function only accessible if `msg.data.length == 0`.
  *
  * WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
  * Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
  * recovery.
  */
📝 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.

Suggested change
/**
* @dev Context variant with ERC-2771 support.
*
* WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll
* be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC-2771
* specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
* behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
* function only accessible if `msg.data.length == 0`.
*
* WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
* Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
* recovery.
*/
abstract contract ERC2771ContextCustom is Context, TransferrableOwnership {
/**
* @custom:version 1.0.0
* @dev Context variant with ERC-2771 support.
*
* WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll
* be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC-2771
* specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
* behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
* function only accessible if `msg.data.length == 0`.
*
* WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
* Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
* recovery.
*/
abstract contract ERC2771ContextCustom is Context, TransferrableOwnership {
🧰 Tools
🪛 GitHub Actions: Version Control Checker

[error] Missing custom:version tag in the contract code. Every version-controlled contract needs to have a custom:version tag.

Comment on lines +25849 to +25854
"ADDRESS": "0xb33Fe241BEd9bf5F694101D7498F63a0d060F999",
"OPTIMIZER_RUNS": "1000000",
"TIMESTAMP": "2025-01-22 07:56:36",
"CONSTRUCTOR_ARGS": "0x000000000000000000000000d3b2b0ac0afdd0d166a495f5e9fca4ecc715a782000000000000000000000000000000000022d473030f116ddee9f6b43ac78ba30000000000000000000000009e606d0d2bba344b911e2f4eab95d9235a83fe1500000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000001000000000000000000000000d8253782c45a12053594b9deb72d8e8ab2fca54c",
"SALT": "",
"VERIFIED": "false"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Production contract needs verification.

The contract at 0xb33Fe241BEd9bf5F694101D7498F63a0d060F999 is not verified in the production environment. This makes it difficult to verify the deployed bytecode matches the source code.

Please verify the contract on the blockchain explorer to ensure transparency and security.

@lifi-action-bot
Copy link
Collaborator

Test Coverage Report

Line Coverage: 78.48% (2266 / 2887 lines)
Function Coverage: 84.73% ( 394 / 465 functions)
Branch Coverage: 39.64% ( 224 / 565 branches)
Test coverage (78.48%) is above min threshold (76%). Check passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants