Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L139-L158

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

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

Both functions query the callback after the transfer: 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,

            // 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"
        );
    }

This is a classic implementation of ensuring safe transfers for NFT's and in this case it's used by calling the callback onERC721Receivedin the recipient if they have codesize > 0. Per the documentation and as expected, if the callback doesn't return the selector, the function should revert.

Issue however is that the opposite is being checked for in the function, see https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L91-L95


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

Impact

Intended functionality is broken, considering any function/contracts that needs to query this to execute it's flow would always revert here.

Considering contracts are expected to integrate protocol who depend on the validity from the onERC721Received selector, this leads to often reverts.

Recommended Mitigation Steps

Apply these changes:


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

Assessed type

Token-Transfer

code-423n4 / 2024-08-superposition-validation