Skip to content

Remove unreachable return value checks in ERC1363 #6206

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.

Sign up for GitHub

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

Open
anim001k wants to merge 1 commit into
base: master
Choose a base branch
from
+3 −9
Open

Remove unreachable return value checks in ERC1363 #6206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions contracts/token/ERC20/extensions/ERC1363.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 {
* no specified format.
*/
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;
}
Comment on lines 67 to 71
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 14, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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.sol

Repository: 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 -100

Repository: 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 -20

Repository: 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.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 2610

🏁 Script executed:

# Get the _transfer function implementation
sed -n '159,220p' contracts/token/ERC20/ERC20.sol

Repository: 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 solidity

Repository: 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.sol

Repository: 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 --stat

Repository: 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.js

Repository: 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.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 61

🏁 Script executed:

# Check ERC1363Utils to understand callback validation
cat contracts/token/ERC20/utils/ERC1363Utils.sol

Repository: 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.

Expand Down Expand Up @@ -98,9 +96,7 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 {
uint256 value,
bytes memory data
) public virtual returns (bool) {
if (!transferFrom(from, to, value)) {
revert ERC1363TransferFromFailed(from, to, value);
}
transferFrom(from, to, value);
ERC1363Utils.checkOnERC1363TransferReceived(_msgSender(), from, to, value, data);
return true;
}
Expand All @@ -126,9 +122,7 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 {
* no specified format.
*/
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;
}
Comment on lines 124 to 128
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 14, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the ERC1363.sol file and surrounding context
cd contracts && find . -name "ERC1363.sol" -type f

Repository: 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 -5

Repository: 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 -50

Repository: 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 -20

Repository: 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.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 127

🏁 Script executed:

# Read the relevant section with context
sed -n '110,135p' contracts/token/ERC20/extensions/ERC1363.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 929

🏁 Script executed:

# Check the documentation comment more carefully
sed -n '100,135p' contracts/token/ERC20/extensions/ERC1363.sol

Repository: 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 -3

Repository: 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 -3

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 163

🏁 Script executed:

# Look for ERC1363 test files specifically
find . -path "*/test/*" -name "*ERC1363*" -type f 2>/dev/null

Repository: 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 5

Repository: 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.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 1282

🏁 Script executed:

# Check the test file size first
wc -l test/token/ERC20/extensions/ERC1363.test.js

Repository: 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 -40

Repository: 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 -20

Repository: 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.js

Repository: 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.sol

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 2300

🏁 Script executed:

# Check the IERC1363 interface documentation
head -60 contracts/interfaces/IERC1363.sol

Repository: 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 -10

Repository: 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 -40

Repository: 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.sol

Repository: 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/null

Repository: 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.sol

Repository: 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.sol

Repository: 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.sol

Repository: 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 -20

Repository: 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.

Expand Down
Loading