There is a critical bug in the _onTransferReceived function. The function incorrectly checks the return value from the onERC721Received callback, causing all safe transfers to contract addresses to fail, even when the receiving contract correctly implements the ERC721 receiver interface.
function _onTransferReceived(
address _sender,
address _from,
address _to,
uint256 _tokenId
) internal {
if (_to.code.length == 0) return;
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
""
);
require(
data != IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);
}
The bug is in the require statement. It's checking if the returned data is NOT equal to the expected selector, which is the opposite of what it should do.
Impact
All safe transfers to contract addresses will fail, even if the receiving contract correctly implements onERC721Received.
Users will be unable to transfer NFTs to smart contract wallets or other contracts designed to hold NFTs.
This severely limits the functionality and interoperability of the NFT contract.
Scenario
A user attempts to transfer an NFT to a smart contract wallet using safeTransferFrom.
The smart contract wallet correctly implements onERC721Received and returns the expected selector.
The transfer fails because the require statement in _onTransferReceived reverts the transaction.
Fix
The fix is to change the != operator to == in the require statement:
require(
data == IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);
This change ensures that the function only proceeds if the receiving contract returns the correct selector, indicating it can handle ERC721 tokens.
Lines of code
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L73
Vulnerability details
Summary
There is a critical bug in the
_onTransferReceived
function. The function incorrectly checks the return value from theonERC721Received
callback, causing all safe transfers to contract addresses to fail, even when the receiving contract correctly implements the ERC721 receiver interface.Code Snippet
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L73
The bug is in the
require
statement. It's checking if the returned data is NOT equal to the expected selector, which is the opposite of what it should do.Impact
onERC721Received
.Scenario
safeTransferFrom
.onERC721Received
and returns the expected selector.require
statement in_onTransferReceived
reverts the transaction.Fix
The fix is to change the
!=
operator to==
in therequire
statement:This change ensures that the function only proceeds if the receiving contract returns the correct selector, indicating it can handle ERC721 tokens.
Assessed type
Invalid Validation