Incorrect onERC721Received Selector Check in _onTransferReceived()

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The incorrect check in _onTransferReceived() prevents NFTs from being transferred to contracts that correctly implement the ERC721 token receiver interface.

Proof of Concept

The _onTransferReceived() function is designed to ensure that when an NFT is transferred to a contract, the recipient contract correctly implements the onERC721Received function. This function should return a specific selector to confirm that it can handle ERC721 tokens.

    // if the callback doesn't return the selector, revert!
    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"
        );
    }

However, the current implementation incorrectly checks for inequality rather than equality.

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

This means that the function will revert if the recipient contract correctly returns the expected selector, which is the opposite of the intended behavior. As a result, any contract that correctly implements the onERC721Received function will be unable to receive NFTs from this contract.

Tools Used

Manual Review

Recommended Mitigation Steps

The require statement should be corrected to check for equality, ensuring that the recipient contract's onERC721Received function returns the expected selector.

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

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as satisfactory