LF-12880 Added new error case [Permit2Proxy v1.0.3] #1080
Conversation
## Walkthrough
The changes introduce an enhanced error handling mechanism in the `Permit2Proxy` contract's `callDiamondWithEIP2612Signature` function, specifically addressing byte-based errors. A new abstract contract `BaseMockPermitToken` and a concrete implementation `MockPermitToken` are added for testing, which simulate various error conditions related to the permit functionality. This includes a custom error type, `CustomPermitError`, and multiple test functions that validate the expected behavior when the permit call fails under different scenarios. Additionally, an audit entry for the `Permit2Proxy` contract version 1.0.3 was added, documenting a completed audit.
## Changes
| File(s) | Change Summary |
|------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `src/Periphery/Permit2Proxy.sol` | Introduced a new catch block in `callDiamondWithEIP2612Signature` to handle `bytes` errors by checking token allowance and reverting with the original error message if the allowance is insufficient. |
| `test/solidity/Periphery/Permit2Proxy.t.sol` | Added abstract contract `BaseMockPermitToken` and concrete contract `MockPermitToken`, which includes an overridden `permit` function. Introduced multiple mock contracts for testing various revert scenarios and added several test functions to validate error handling. |
| `audit/auditLog.json` | Added a new audit entry "audit20250415" for the `Permit2Proxy` contract version 1.0.3, including auditor details, report path, and commit hash. |
## Possibly related PRs
- lifinance/contracts#910: The main PR extends the error handling in `callDiamondWithEIP2612Signature` by adding a catch block for bytes errors and introduces comprehensive tests for various revert scenarios, building directly upon the error handling improvements introduced in the retrieved PR #910 for the same function in `Permit2Proxy.sol`.
- lifinance/contracts#912: The main PR enhances error handling in the `callDiamondWithEIP2612Signature` function of `Permit2Proxy.sol` by adding a `try-catch` block for `permit` calls and introduces extensive tests for various revert scenarios, while the retrieved PR #912 also modifies `Permit2Proxy.sol` but focuses primarily on replacing low-level ETH transfers with `SafeTransferLib.safeTransferETH` and adding a `try-catch` for `permit` calls without the detailed revert scenario tests; thus, both PRs modify the same function and contract with overlapping error handling improvements, making them related.
## Suggested labels
`AuditCompleted`
## Suggested reviewers
- 0xDEnYOWarning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
Test Coverage ReportLine Coverage: 79.67% (2113 / 2652 lines) |
lifi-action-bot
changed the title
mirooon
force-pushed
the
lf-12880-handle-all-possible-revert-cases-in-permit2proxy
branch
from
d643b11 to
b378d5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/solidity/Periphery/Permit2Proxy.t.sol (1)
709-730: Comment mismatch regarding the amount triggeringrequire.
Although the comment says “Amount less than 1000 to trigger require,” the actual revert isrequire(true == false), which ignores the input entirely. Consider aligning the commented explanation with the actual logic, or switching the require condition to use the supplied amount if that is the intended test scenario.- 500, // Amount less than 1000 to trigger require + 500, // This value is not used in the require logic; consider removing or reusing it
🛑 Comments failed to post (1)
test/solidity/Periphery/Permit2Proxy.t.sol (1)
732-753: 🛠️ Refactor suggestion
Test name does not match the overflow scenario.
The test is namedtestRevert_FailsOnDivisionByZero, but the contract triggers an addition overflow. Update the test name or the contract logic for consistency.-function testRevert_FailsOnDivisionByZero() public { +function testRevert_FailsOnAdditionOverflow() public {📝 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 testRevert_FailsOnAdditionOverflow() public { MockPermitTokenAdditionOverflow token = new MockPermitTokenAdditionOverflow(); address tokenAddress = address(token); vm.startPrank(permit2User); bytes memory callData = _getCalldataForBridging(); // Expect arithmetic overflow panic vm.expectRevert(stdError.arithmeticError); permit2Proxy.callDiamondWithEIP2612Signature( tokenAddress, defaultUSDCAmount, block.timestamp + 1000, 27, // dummy v bytes32(0), // dummy r bytes32(0), // dummy s callData ); vm.stopPrank(); }
…s, including custom errors, panic assertions, empty reverts, string reverts, require statements and overflow addtion (audit issue#1) created multiple mock token contracts to test various revert scenarios, including custom errors, panic assertions, empty reverts, string reverts, require statements and overflow addtion (audit issue#1)
mirooon
force-pushed
the
lf-12880-handle-all-possible-revert-cases-in-permit2proxy
branch
from
a9b1729 to
5b212bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/solidity/Periphery/Permit2Proxy.t.sol (2)
13-36: Abstract contract structure is well-defined.Clearly separating the core ERC20 + ERC20Permit functionality in an abstract base is commendable and makes mocking easier. Consider removing
solhint-disable-next-line no-unused-varsif no longer needed.
624-646: Custom error revert test is cohesive.The test effectively expects the correct custom error revert for
MockPermitToken. If the reference to “Allowance” in the name is unneeded, consider a clearer function name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/solidity/Periphery/Permit2Proxy.t.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (15)
test/solidity/Periphery/Permit2Proxy.t.sol (15)
4-6: Imports look appropriate.These additional imports from OpenZeppelin and local TestBase are valid and align with the usage below.
11-11: Use ofstdErroris a good approach.Referencing
stdErrorfromforge-std/Test.solis helpful for verifying specific panic and arithmetic reverts.
38-45: Custom error usage is clear.Throwing a custom error in
_permit()effectively tests revert scenarios. Implementation is straightforward.
46-52: Panic test token is correct.Forcing an assertion failure is the right way to simulate a panic revert in Foundry.
53-58: Empty revert logic is sound.This contract properly simulates a revert with no reason.
60-66: String revert test is well-structured.Including a custom message ensures coverage for string-based revert scenarios.
67-73: Require-based revert is clearly demonstrated.The
require(true == false)exemplifies a typical logical revert scenario.
74-80: Overflow scenario is adequately modeled.Using
type(uint256).max + 1checks arithmetic overflow handling.
81-88: Division by zero scenario is valid.Explicitly dividing by zero is a suitable test for arithmetic revert conditions.
648-670: Panic during allowance simulation is effective.Testing with
MockPermitTokenPanicaccurately captures assertion-based panics.
672-693: Empty revert scenario is well-tested.Verifying an untyped revert ensures coverage of minimal revert contexts.
695-716: String revert scenario is validated correctly.Matching revert messages with
vm.expectRevert("Custom error message")is the recommended approach.
718-739: Require revert is correct.Unconditional require statements confirm the revert reason is checked properly.
741-762: Arithmetic overflow scenario test is comprehensive.Using
stdError.arithmeticErrorensures the test captures any overflows triggered by addition.
764-785: Division by zero scenario test is appropriate.
stdError.divisionErrorprecisely validates a zero divisor revert.
0xDEnYO
deleted the
lf-12880-handle-all-possible-revert-cases-in-permit2proxy
branch
Which Jira task belongs to this PR?
LF-12880
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)