Remove unreachable return value checks in ERC1363 #6206
Conversation
|
WalkthroughThis pull request removes explicit revert-based error handling from three functions in the ERC1363 extension. The affected functions now directly call their underlying transfer, transferFrom, or approve operations, then proceed to ERC1363Utils callback checks and return true. Error handling is delegated to the inner function's revert behavior rather than being managed inline. Post-operation ERC1363 callback checks remain in place. No changes to public function signatures or exported APIs were made; modifications are limited to internal control flow logic. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/token/ERC20/extensions/ERC1363.sol (1)
93-102: ChecktransferFromreturn value before executing callback.
ERC1363 ignores the return value fromtransferFrom(from, to, value)at line 99, relying on revert-on-failure semantics from standard ERC20. However, a derived token that returnsfalseinstead of reverting could allow the callback to execute despite transfer failure. The contract documents this requirement in NatSpec ("The internal {transferFrom} must succeed") but doesn't enforce it programmatically. Add an explicit check:require(transferFrom(from, to, value), ...)or use the unusedERC1363TransferFromFailederror, similar to howSafeERC20handles this pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC20/extensions/ERC1363.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
📚 Learning: 2025-10-03T13:14:57.679Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
Applied to files:
contracts/token/ERC20/extensions/ERC1363.sol
⏰ 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). (6)
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: tests-upgradeable
- GitHub Check: tests-foundry
- GitHub Check: halmos
| function transferAndCall(address to, uint256 value, bytes memory data) public virtual returns (bool) { | ||
| if (!transfer(to, value)) { | ||
| revert ERC1363TransferFailed(to, value); | ||
| } | ||
| transfer(to, value); | ||
| ERC1363Utils.checkOnERC1363TransferReceived(_msgSender(), _msgSender(), to, value, data); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full ERC1363.sol file to understand context and NatSpec
cat -n contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 6388
🏁 Script executed:
# Check the ERC20.sol base implementation to verify revert vs return false behavior
head -150 contracts/token/ERC20/ERC20.sol | tail -100Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 3330
🏁 Script executed:
# Check the _transfer implementation to see if it reverts on failure
grep -n "_transfer" contracts/token/ERC20/ERC20.sol | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 238
🏁 Script executed:
# Let's look at the _transfer function specifically
sed -n '240,330p' contracts/token/ERC20/ERC20.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 2610
🏁 Script executed:
# Get the _transfer function implementation
sed -n '159,220p' contracts/token/ERC20/ERC20.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 2357
🏁 Script executed:
# Search for usage of the error types to see if they're used anywhere
rg "ERC1363TransferFailed|ERC1363TransferFromFailed|ERC1363ApproveFailed" --type solidityRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 111
🏁 Script executed:
# Check git history to see if these error types were recently added/removed
git log --oneline -20 -- contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 106
🏁 Script executed:
# Search for usage of the error types without type restriction
rg "ERC1363TransferFailed|ERC1363TransferFromFailed|ERC1363ApproveFailed"Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 783
🏁 Script executed:
# Check the recent commit that updated ERC1363.sol
git show d473269d --statRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 50391
🏁 Script executed:
# Check the ERC1363 test file to see what behavior is tested
head -100 test/token/ERC20/extensions/ERC1363.test.jsRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 3539
🏁 Script executed:
# Check SafeERC20 to see if it uses the error types
grep -A 5 -B 5 "ERC1363TransferFailed\|ERC1363TransferFromFailed\|ERC1363ApproveFailed" contracts/token/ERC20/utils/SafeERC20.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 61
🏁 Script executed:
# Check ERC1363Utils to understand callback validation
cat contracts/token/ERC20/utils/ERC1363Utils.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 3375
Document expected behavior for transfer/transferFrom/approve overrides, or add assertions.
If a derived contract overrides transfer() (line 68), transferFrom() (line 99), or approve() (line 125) to return false without reverting, the ERC1363 functions will still execute the callback and return true, misleading the caller. The NatSpec says "must succeed (returned true)" but the code no longer checks return values. Either document that only revert-on-failure overrides are supported, or add assertions/requires to enforce this expectation.
🤖 Prompt for AI Agents
In contracts/token/ERC20/extensions/ERC1363.sol around lines 67-71 (and
similarly for the transferFrom override at ~line 99 and approve at ~line 125),
the wrapper functions call transfer/transferFrom/approve but do not assert their
boolean return value, so a derived contract that returns false without reverting
can cause the ERC1363 callback to run and the wrapper to return true; change the
wrappers to capture the return value of transfer/transferFrom/approve and
require it to be true (revert with a clear message) before invoking the ERC1363
callback and returning, or alternatively document in the NatSpec that overrides
must revert on failure and add explicit require statements enforcing that
behavior.
| function approveAndCall(address spender, uint256 value, bytes memory data) public virtual returns (bool) { | ||
| if (!approve(spender, value)) { | ||
| revert ERC1363ApproveFailed(spender, value); | ||
| } | ||
| approve(spender, value); | ||
| ERC1363Utils.checkOnERC1363ApprovalReceived(_msgSender(), spender, value, data); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the ERC1363.sol file and surrounding context
cd contracts && find . -name "ERC1363.sol" -type fRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 115
🏁 Script executed:
# Let's also check the OpenZeppelin ERC20 implementation to understand approve() behavior
find . -name "ERC20.sol" -type f | head -5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 112
🏁 Script executed:
# Search for any documentation about ERC1363 and the semantics of approveAndCall
rg -A 10 -B 5 "function approveAndCall" contracts/Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 7351
🏁 Script executed:
# Check the ERC1363Utils to understand what checkOnERC1363ApprovalReceived does
rg -A 15 "function checkOnERC1363ApprovalReceived" contracts/Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1387
🏁 Script executed:
# Let's examine OpenZeppelin's ERC20.approve to understand its return value semantics
rg -A 10 "function approve" contracts/ -B 2 | head -50Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 3572
🏁 Script executed:
# Check if ERC20.approve is marked final
rg -A 5 "function approve" contracts/token/ERC20/ERC20.sol | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 270
🏁 Script executed:
# Let's read the full ERC1363.sol file to understand all context
wc -l contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 127
🏁 Script executed:
# Read the relevant section with context
sed -n '110,135p' contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 929
🏁 Script executed:
# Check the documentation comment more carefully
sed -n '100,135p' contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 1315
🏁 Script executed:
# Search for tests related to approveAndCall to see what behavior is tested
find . -name "*.test.ts" -o -name "*.test.js" -o -name "*.test.sol" | xargs grep -l "approveAndCall" 2>/dev/null | head -3Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 167
🏁 Script executed:
# Check if there are any test files that test custom approve() behavior
fd -e "test.ts" -e "test.js" | xargs grep -l "ERC1363" 2>/dev/null | head -3Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 163
🏁 Script executed:
# Look for ERC1363 test files specifically
find . -path "*/test/*" -name "*ERC1363*" -type f 2>/dev/nullRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 124
🏁 Script executed:
# Search for any comments about the approve() return value handling in the codebase
rg "approve.*return" contracts/token/ERC20/extensions/ERC1363.sol -B 5 -A 5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 950
🏁 Script executed:
# Check what the transferAndCall function does for comparison (does it check return value?)
sed -n '80,110p' contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 1282
🏁 Script executed:
# Check the test file size first
wc -l test/token/ERC20/extensions/ERC1363.test.jsRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 126
🏁 Script executed:
# Search for tests related to approve or custom implementations in the test file
rg -A 10 "describe.*approve|test.*approve" test/token/ERC20/extensions/ERC1363.test.js | head -40Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 443
🏁 Script executed:
# Look for mock usage in tests
rg "Mock|mock" test/token/ERC20/extensions/ERC1363.test.js | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 403
🏁 Script executed:
# Check if the test file tests the return value handling
rg "return.*false|ReturnFalse" test/token/ERC20/extensions/ERC1363.test.jsRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 61
🏁 Script executed:
# Read the full ERC1363.sol file to see the class declaration and documentation
head -50 contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 2300
🏁 Script executed:
# Check the IERC1363 interface documentation
head -60 contracts/interfaces/IERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 3037
🏁 Script executed:
# Look at the context around the issue - is this related to a specific change or bug report?
git log --oneline -n 20 -- contracts/token/ERC20/extensions/ERC1363.sol 2>/dev/null | head -10Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 106
🏁 Script executed:
# Check if ERC1363ApproveFailed is defined but truly never used
rg "ERC1363ApproveFailed" contracts/Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 187
🏁 Script executed:
# Check the git history for when this error was added
git log -p -- contracts/token/ERC20/extensions/ERC1363.sol 2>/dev/null | grep -A 5 -B 5 "ERC1363ApproveFailed" | head -40Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 61
🏁 Script executed:
# Check if there are any similar errors that ARE being used
rg "ERC1363TransferFailed|ERC1363TransferFromFailed" contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 230
🏁 Script executed:
# Let's check when these errors were added and look at recent changes to the file
git log --oneline --all -- contracts/token/ERC20/extensions/ERC1363.sol 2>/dev/nullRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 624
🏁 Script executed:
# Let's see the actual implementation to confirm none of the errors are used
grep -n "ERC1363ApproveFailed\|ERC1363TransferFailed\|ERC1363TransferFromFailed" contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 303
🏁 Script executed:
# Check if checkOnERC1363ApprovalReceived could throw and would that be sufficient
rg -A 20 "function checkOnERC1363ApprovalReceived" contracts/token/ERC20/utils/ERC1363Utils.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 865
🏁 Script executed:
# Look for any comments or documentation about why these errors exist but aren't used
rg -B 10 -A 2 "error ERC1363ApproveFailed" contracts/token/ERC20/extensions/ERC1363.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 605
🏁 Script executed:
# Check if there's any documentation about supported ERC20 implementations
rg -i "erc20|override|compatible" contracts/token/ERC20/extensions/ERC1363.sol | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 471
Check the approve return value or document that only OZ-compliant ERC20s are supported.
The approveAndCall function at line 125 ignores the return value of approve(spender, value). While OpenZeppelin's standard ERC20 reverts on failure rather than returning false, this contract is abstract and designed for extension. If a subclass overrides approve() with non-standard semantics (returning false without reverting), the function will still invoke checkOnERC1363ApprovalReceived() and return true despite the approval failing. The presence of the unused ERC1363ApproveFailed error suggests this scenario was anticipated. Either validate the return value before proceeding, or document that only OZ ERC20-compliant (revert-on-failure) implementations are supported.
🤖 Prompt for AI Agents
In contracts/token/ERC20/extensions/ERC1363.sol around lines 124 to 128, the
approveAndCall function ignores the return value of approve(spender, value)
which can allow silent failure for non‑standard ERC20 implementations; change
the flow to capture the boolean result of approve and validate it before calling
ERC1363Utils.checkOnERC1363ApprovalReceived (i.e., require or revert with
ERC1363ApproveFailed when approve returns false), then call the
approval-receiver check and return true only if the approval succeeded.
1 participant
Remove unreachable return value checks in
transferAndCall,transferFromAndCall, andapproveAndCallfunctions.The base
ERC20contract'stransfer,transferFrom, andapprovefunctions always revert on failure and never returnfalse. Therefore, the conditional checksif (!transfer(...)) revertare unreachable dead code that only increases bytecode size and gas costs without providing any functional benefit.