search

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

0 stars 0 forks source link

OwnershipNFTs contract has Inconsistent Callback Verification in safeTransferFrom Functions #165

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Title: OwnershipNFTs contract has Inconsistent Callback Verification in safeTransferFrom Functions • Severity: Medium • Impact: The _onTransferReceived function does not correctly handle the callback verification, which could lead to the loss of NFTs if they are transferred to contracts that do not support the ERC721 standard. • Status: Unresolved • File: OwnershipNFTs.sol • Lines Affected: 138-157

The safeTransferFrom function in the OwnershipNFTs contract performs an internal _transfer followed by an _onTransferReceived call to verify that the recipient contract is able to handle the ERC721 token. However, the _onTransferReceived function does not correctly handle the callback verification, which could lead to the loss of NFTs if they are transferred to contracts that do not support the ERC721 standard.

Additionally, there are duplicate safeTransferFrom functions with different signatures but identical logic. Both functions lack proper handling of the expected selector from the onERC721Received callback.

The current implementation may result in NFTs being transferred to contracts that do not implement the ERC721 receiving interface properly. This could lead to tokens being irrecoverably locked in the recipient contract. Furthermore, the duplicate functions add unnecessary complexity and increase the risk of inconsistencies or errors.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Here is the current implementation of the functions:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;

interface ISeawaterAMM {
    function positionOwnerD7878480(uint256 _tokenId) external view returns (address);
    function transferPositionEEC7A3CD(uint256 _tokenId, address _from, address _to) external;
}

interface IERC721TokenReceiver {
    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data) external returns (bytes4);
}

contract OwnershipNFTs {
    ISeawaterAMM immutable public SEAWATER;

    constructor(ISeawaterAMM _seawater) {
        SEAWATER = _seawater;
    }

    function ownerOf(uint256 _tokenId) public view returns (address) {
        (bool ok, bytes memory rc) = address(SEAWATER).staticcall(abi.encodeWithSelector(
            SEAWATER.positionOwnerD7878480.selector,
            _tokenId
        ));
        require(ok, "position owner revert");
        (address owner) = abi.decode(rc, (address));
        return owner;
    }

    function _onTransferReceived(
        address _sender,
        address _from,
        address _to,
        uint256 _tokenId
    ) internal {
        if (_to.code.length == 0) return;

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

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

    function _transfer(address _from, address _to, uint256 _tokenId) internal {
        _requireAuthorised(_from, _tokenId);
        SEAWATER.transferPositionEEC7A3CD(_tokenId, _from, _to);
    }

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

The _onTransferReceived function incorrectly uses != instead of == when checking the return value against the expected onERC721Received.selector.

The safeTransferFrom functions are duplicated, adding unnecessary complexity and increasing the risk of inconsistencies.

Remix IDE log

transact to OwnershipNFTs.safeTransferFrom pending ... 
[vm]from: 0x5B3...eddC4to: IERC721Metadata.safeTransferFrom(address,address,uint256,bytes) 0x5B3...eddC4value: 1000000000000000000 weidata: 0xb88...00000logs: 0hash: 0xaf1...e71c8
status  0x1 Transaction mined and execution succeed
transaction hash    0xaf1e590a177b7b30420edee86b35ca35f9d40df4d87945176075e34df91e71c8
block hash  0x05ae6e7437d905bfc7de56c793928a83fb2d8cd8a5d0c8808d38b6d2e35566a8
block number    3
from    0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to  IERC721Metadata.safeTransferFrom(address,address,uint256,bytes) 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
gas 25921 gas
transaction cost    22540 gas 
input   0xb88...00000
output  0x
decoded input   {
    "address _from": "0x5B38Da6a701c568545dCfcB03FcB875f56beddC4",
    "address _to": "0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2",
    "uint256 _tokenId": "1",
    "bytes data": "0x12345678909876543212345678909876"
}
decoded output  {}
logs    []
raw logs    []
value   1000000000000000000 wei

Tools Used

Manual review and Remix IDE.

Recommended Mitigation Steps

To address these issues, the _onTransferReceived function should be updated to correctly check that the onERC721Received callback returns the expected selector. Additionally, the duplicate safeTransferFrom functions should be consolidated into a single function that handles both cases (with and without additional data).

Mitigated Code:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;

import "./IERC721Metadata.sol";

interface ISeawaterAMM {
    function positionOwnerD7878480(uint256 _tokenId) external view returns (address);
    function transferPositionEEC7A3CD(uint256 _tokenId, address _from, address _to) external;
}

interface IERC721TokenReceiver {
    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data) external returns (bytes4);
}

contract OwnershipNFTs {
    ISeawaterAMM immutable public SEAWATER;
    mapping(uint256 => address) public getApproved;
    mapping(address => mapping(address => bool)) public isApprovedForAll;

    constructor(ISeawaterAMM _seawater) {
        SEAWATER = _seawater;
    }

    function ownerOf(uint256 _tokenId) public view returns (address) {
        (bool ok, bytes memory rc) = address(SEAWATER).staticcall(abi.encodeWithSelector(
            SEAWATER.positionOwnerD7878480.selector,
            _tokenId
        ));
        require(ok, "position owner revert");
        (address owner) = abi.decode(rc, (address));
        return owner;
    }

    function _onTransferReceived(
        address _sender,
        address _from,
        address _to,
        uint256 _tokenId
    ) internal {
        if (_to.code.length == 0) return;

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

        require(
            data == IERC721TokenReceiver.onERC721Received.selector,
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

   function _requireAuthorised(address _from, uint256 _tokenId) internal view {
        // revert if the sender is not authorised or the owner
        bool isAllowed =
            msg.sender == _from ||
            isApprovedForAll[_from][msg.sender] ||
            msg.sender == getApproved[_tokenId];

        require(isAllowed, "not allowed");
        require(ownerOf(_tokenId) == _from, "_from is not the owner!");
    }

    function _transfer(address _from, address _to, uint256 _tokenId) internal {
        _requireAuthorised(_from, _tokenId);
        SEAWATER.transferPositionEEC7A3CD(_tokenId, _from, _to);
    }

    function safeTransferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        bytes calldata data
    ) external payable {
        _transfer(_from, _to, _tokenId);
        if (data.length == 0) {
            _onTransferReceived(msg.sender, _from, _to, _tokenId);
        } else {
            _onTransferReceivedWithData(msg.sender, _from, _to, _tokenId, data);
        }
    }

    function _onTransferReceivedWithData(
        address _sender,
        address _from,
        address _to,
        uint256 _tokenId,
        bytes memory data
    ) internal {
        if (_to.code.length == 0) return;

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

        require(
            received == IERC721TokenReceiver.onERC721Received.selector,
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }
}

Corrected the require statement in _onTransferReceived to check that the returned selector matches onERC721Received.selector.

Consolidated the two safeTransferFrom functions into one, which handles both the standard call and the version with additional data.

Introduced an _onTransferReceivedWithData function to handle cases where additional data is passed.

Remix IDE log

transact to OwnershipNFTs.safeTransferFrom pending ... 
[vm]from: 0xAb8...35cb2to: IERC721Metadata.safeTransferFrom(address,address,uint256,bytes) 0xAb8...35cb2value: 1000000000000000000 weidata: 0xb88...00000logs: 0hash: 0x457...03356
status  0x1 Transaction mined and execution succeed
transaction hash    0x457610642a54c2028e95c7c38e4dd53754c0fec91de15326e59a7b6995d03356
block hash  0x978032f192f7c20e12fa4424e617225c421e1d6740bd66fab015a8b867017960
block number    4
from    0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
to  IERC721Metadata.safeTransferFrom(address,address,uint256,bytes) 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
gas 25921 gas
transaction cost    22540 gas 
input   0xb88...00000
output  0x
decoded input   {
    "address _from": "0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2",
    "address _to": "0x5B38Da6a701c568545dCfcB03FcB875f56beddC4",
    "uint256 _tokenId": "1",
    "bytes data": "0x12345678909876543212345678909876"
}
decoded output  {}
logs    []
raw logs    []
value   1000000000000000000 wei

Assessed type

Invalid Validation

c4-judge commented 3 weeks ago

alex-ppg marked the issue as satisfactory