bytes data param is not passed to ERC721 recipient as expected by EIP-721

[howlbot-integration]

Lines of code

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

Vulnerability details

Bug Description

ERC-721 standard has two variations of the safeTransferFrom(), one with no bytes data param and the other with a bytes data param. The second variation that accepts a bytes data param is meant to pass that data param to the token recipient during the IERC721TokenReceiver. onERC721Received() call. This bytes data param is usually an encoding of extra variables which is used by the recipient contract for the extra logic it will perform as it receives a token.

OwnershipNFTs.sol is ERC721 compliant and so it has two safeTransferFrom() functions but the second safeTransferFrom() has a bytes data param which is not passed into the recipient during IERC721TokenReceiver. onERC721Received() call to recipient. It accepts the bytes data param but doesnt use it.

    /// @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);
    

As we can see above the bytes param is declared as an arbitrary param, perhaps just to conform to IERC721 spec but the bytes param is not used in the logic at all, and it ought to be used as defined here in the EIP.

/// @param data Additional data with no specified format, sent in call to `_to`

For contracts that integrate OwnershipNFTs or receive transfers, if they have logic which require the additional bytes passed in via the second safeTransferFrom(), they will be unable to execute this logic and may reject the transfers or revert as this bytes data supplied by the caller of the safeTransferFrom() is not passed into them via the IERC721TokenReceiver.onERC721Received() call. Instead, empty bytes is passed into IERC721TokenReceiver.onERC721Received() as seen here.

Below is a POC which shows how passing empty bytes by default instead of passing the bytes specified by the sender into a tokenReceiver may cause the transfer to fail.

• run with forge test

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";

/// @dev Note: the ERC-165 identifier for this interface is 0x150b7a02.
interface IERC721TokenReceiver {
    /// @notice Handle the receipt of an NFT
    /// @dev The ERC721 smart contract calls this function on the recipient
    ///  after a `transfer`. This function MAY throw to revert and reject the
    ///  transfer. Return of other than the magic value MUST result in the
    ///  transaction being reverted.
    ///  Note: the contract address is always the message sender.
    /// @param _operator The address which called `safeTransferFrom` function
    /// @param _from The address which previously owned the token
    /// @param _tokenId The NFT identifier which is being transferred
    /// @param _data Additional data with no specified format
    /// @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
    ///  unless throwing
    function onERC721Received(
        address _operator,
        address _from,
        uint256 _tokenId,
        bytes memory _data
    ) external returns (bytes4);
}

/*
 * OwnershipNFTs is a simple interface for tracking ownership of
 * positions in the Seawater Stylus contract.
 */
contract OwnershipNFTs {
    /**
     * @notice _onTransferReceived by calling the callback `onERC721Received`
     *         in the recipient if they have codesize > 0. if the callback
     *         doesn't return the selector, revert!
     * @param _sender that did the transfer
     * @param _from owner of the NFT that the sender is transferring
     * @param _to recipient of the NFT that we're calling the function on
     * @param _tokenId that we're transferring from our internal storage
     */
    //  _onTransferReceived() is exact same function that is in the codebase, with no changes to logic whatsoever
    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
            ""
        );

        //@audit we dont expect execution to get here, revert should happen in the call to _to because of decodiing of empty bytes in _to logic
        // require(
        //     data != IERC721TokenReceiver.onERC721Received.selector,
        //     "bad nft transfer received data"
        // );
    }

    //the function below is same as seen here -> https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L148-L157
    function safeTransferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        bytes calldata /** _data */
    ) external {
        //_transfer(_from, _to, _tokenId);
        _onTransferReceived(msg.sender, _from, _to, _tokenId);
    }
}

contract MockCompliantReceiver is IERC721TokenReceiver {
    function onERC721Received(
        address _operator,
        address _from,
        uint256 _tokenId,
        bytes memory _data
    ) external returns (bytes4) {
        //get values in bytes memory _data
        (uint timestamp, uint commandID) = abi.decode(_data, (uint, uint));

        return IERC721TokenReceiver.onERC721Received.selector;
    }
}

contract receiverTest is Test {
    MockCompliantReceiver compliant_receiver;
    OwnershipNFTs ownershipNft;

    function setUp() public {
        //deploy MockCompliantReceiver
        compliant_receiver = new MockCompliantReceiver();

        //deploy ownershipNft contract
        ownershipNft = new OwnershipNFTs();
    }

    function test_revertForCompliantReceiver() public {
        //data to be encoded and expected passed into the recipient contract by a sender/user
        bytes memory data = abi.encode(block.timestamp, 10);


        /* we expect the call to revert because of the decoding of the empty bytes data passed into _to.OnERC721Received() */

        vm.expectRevert();
        ownershipNft.safeTransferFrom(
            msg.sender,
            payable(address(compliant_receiver)), //param `to` is set to be the compliant receiver here
            1,
            data
        );
    }
}

Impact

the implementation of the safeTransferFrom() function that accepts additional bytes is not sufficient and not as defined by the EIP-721 spec. The bytes param passed in by a caller is not sent to the recipient contract. Receiving contracts may need to decode this bytes param, but since empty bytes are passed in to recipient by default in OwnershipNfts this will cause reverts/inaccessibility to this function/feature of the ERC721

Tools Used

manual review

Recommended Mitigation Steps

The issue stems from the _onTransferReceived() not accepting a bytes data param. Modify it to accept it and pass that param into the IERC721TokenReceiver.onERC721Received() call. Then in each safeTransferFrom(), pass the bytes param into the modified _onTransferReceived(). If the safeTransferFrom() accepts no bytes param, pass empty bytes into _onTransferReceived()

    function _onTransferReceived(
        address _sender,
        address _from,
        address _to,
        uint256 _tokenId,
        bytes calldata data
    ) internal {
    ...

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


    /// @inheritdoc IERC721Metadata
    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, _data);
    }

Assessed type

ERC721

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c

[adeolu98]

hi @alex-ppg,

i will like implore you to review your decision on this finding as it is quite different from the findings it was grouped with.

This finding does not just talk about EIP-721 compliance but spotlights an implementation flaw in the callback to the token receiver. The hardcoding of the empty bytes passed to the token receiver when the receiver is a contract obstructs the usecase of the ERC721Receiver callback/hook . The usecases of this hooks are

1. ensure that a contract is ready to receive the erc721 token
2. allow for execution of additional custom logic on receipt of the token.
3. allow for sender to communicate with receiving contract via the bytes data that should be passed into it via the callback. This bytes data will then be used in the additional logic to be executed upon the contract receiving the token.

The OwnershipNFT as it is prevents usecases 2 and 3 and in some cases will may prevent 1 because if a contract that has custom logic which requires bytes to be passed in, it will never be ready to receive the token.

Bytes data passed in by a token sender should be propagated to the token receiver contracts. This is the correct, expected and complete implementation of safeTransferFrom() -> receiver.OnERC721Recevied() callback flow.

[alex-ppg]

Hey @adeolu98, thank you for your PJQA contribution! Indeed, this finding seems to have been grouped incorrectly during the validation phase. Per the rationale laid out in #55, the EIP-721 callback feature was deliberately implemented and does not forward the _data payload as described.

A medium severity rating for this submission similar to #55 is appropriate.

[c4-judge]

This previously downgraded issue has been upgraded by alex-ppg

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory