howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L93

Vulnerability details

There is an incorrect check in the _onTransferReceived function that results in improper validation of the recipient contract when transferring NFTs. Specifically, the function is designed to ensure that the recipient contract implements the onERC721Received interface by checking the returned selector. However, the current implementation incorrectly checks return data.

Impact

The incorrect condition causes two major issues:

Failure to transfer tokens to legitimate contracts: Contracts that correctly implement the ERC-721 interface and return the expected selector will fail the check, preventing legitimate transfers.

Unintentional transfers to non-legitimate contracts: Contracts that do not return the correct selector are allowed to receive NFTs. If these contracts cannot handle the tokens, the NFTs may become irretrievably stuck.

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

    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,
+           data == IERC721TokenReceiver.onERC721Received.selector,
            "bad nft transfer received data"
        );
    }

Assessed type

Error

c4-judge commented 3 weeks ago

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