Inverted Selector Check Causes All ERC721 Safe Transfers to Contract Addresses to Fail

[howlbot-integration]

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 the onERC721Received 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

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

1. All safe transfers to contract addresses will fail, even if the receiving contract correctly implements onERC721Received.
2. Users will be unable to transfer NFTs to smart contract wallets or other contracts designed to hold NFTs.
3. This severely limits the functionality and interoperability of the NFT contract.

Scenario

1. A user attempts to transfer an NFT to a smart contract wallet using safeTransferFrom.
2. The smart contract wallet correctly implements onERC721Received and returns the expected selector.
3. 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.

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as satisfactory