Incorrect ERC721 receiver check causes safe transfers to always fail

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The _onTransferReceived function incorrectly checks the return value from the onERC721Received function call. This causes all safe transfers to contract addresses to fail, even when the receiving contract correctly implements the ERC721 receiver interface.

Proof of Concept

Take a look at the _onTransferReceived function: https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L73-L96

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,
        ""
    );

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

The require statement checks if the returned data is NOT equal to the onERC721Received selector. This is the opposite of what it should be doing. According to the ERC721 standard, the onERC721Received function should return its function selector to indicate successful receipt of the token. but the way it's currently implemented will revert the transaction when the receiver correctly implements the onERC721Received function.

This bug goes on to affect both safeTransferFrom functions in the contract, as they both call _onTransferReceived:

function safeTransferFrom(
    address _from,
    address _to,
    uint256 _tokenId
) external payable {
    _transfer(_from, _to, _tokenId);  <--------
    _onTransferReceived(msg.sender, _from, _to, _tokenId);
}

function safeTransferFrom(
    address _from,
    address _to,
    uint256 _tokenId,
    bytes calldata /* _data */
) external payable {
    _transfer(_from, _to, _tokenId);
    _onTransferReceived(msg.sender, _from, _to, _tokenId);  <--------
}

Tools used

Manual review

Recommended Mitigation Steps

Change the require statement in the _onTransferReceived function to check for equality instead of inequality:

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,
        ""
    );

    require(
-       data != IERC721TokenReceiver.onERC721Received.selector,
-       "bad nft transfer received data"
+       data == IERC721TokenReceiver.onERC721Received.selector,
+       "ERC721: transfer to non ERC721Receiver implementer"
    );
}

Assessed type

Error

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory