Inverted Logic in ERC721 Receiver Check

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L73-L96

Vulnerability details

Vulnerability Details

The _onTransferReceived function has an inverted logic in its check of the returned selector:

require(
    data != IERC721TokenReceiver.onERC721Received.selector,
    "bad nft transfer received data"
);

This check should be == instead of !=.

Impact

This inverted check will cause all valid ERC721 receiver contracts to be rejected, and only invalid ones to be accepted. This breaks the safety check intended to prevent tokens from being sent to contracts that can't handle them.

Proof of Concept

    function _onTransferReceived(
        address _sender,
        address _from,
        address _to,
        uint256 _tokenId
    ) internal {
        // only call the callback if the receiver is a contract
        if (_to.code.length == 0) return;

        bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
            _sender,
            _from,
            _tokenId,

            // this is empty byte data that can be optionally passed to
            // the contract we're confirming is able to receive NFTs
            ""
        );

        require(
            data != IERC721TokenReceiver.onERC721Received.selector,
            "bad nft transfer received data"
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Correct the check to use == instead of !=:

require(
    data == IERC721TokenReceiver.onERC721Received.selector,
    "ERC721: transfer to non ERC721Receiver implementer"
);

Assessed type

ERC721