Incorrect Validation in OwnershipNFTs.sol Prevents Valid NFT Transfers and Allows Invalid Ones

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The bug in the safeTransferFrom function of the OwnershipNFTs contract leads to two critical issues:

1. Denial of Service for Contracts with Correct Implementation: If the recipient (_to address) is a contract that correctly implements the onERC721Received function, the transfer will revert. The root cause is an incorrect conditional check that causes the transfer to fail, even when the contract correctly returns the onERC721Received function selector. This results in a denial of service (DoS) for valid recipient contracts, preventing them from receiving NFTs.

2. Transfer to Incorrectly Implemented Contracts: Conversely, contracts that do not correctly implement the onERC721Received function or return arbitrary values can pass the flawed validation. This means NFTs can be mistakenly transferred to contracts that are not designed to hold NFTs, potentially leading to a loss of assets or other unforeseen consequences due to incorrect handling of NFTs.

These issues have significant implications for the integrity and reliability of the NFT transfer process, impacting both security and user trust in the system.

Severity

• High: The vulnerability directly impacts the correctness of NFT transfers and may result in a loss of assets, disrupt intended functionality, and prevent valid transfers from occurring. The potential loss of user trust and system reliability due to failed transfers further raises the criticality of this issue.

Proof of Concept

Here is a simplified proof of concept (PoC) demonstrating the issue with the OwnershipNFTs contract:

1. Test.t.sol

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

import {Test} from "forge-std/Test.sol";
import {OwnershipNFTs} from "./../src/OwnershipNFTs.sol";
import {SeawaterAMM} from "./../src/SeawaterAMM.sol";
import "./../src/IERC721TokenReceiver.sol";

contract testNft is Test {
    OwnershipNFTs public ownershipnft;
    SeawaterAMM public seawateramm;
    MockContract public mockcontract;

    function setUp() public {
        mockcontract = new MockContract();
        seawateramm = new SeawaterAMM(address(this));
        ownershipnft = new OwnershipNFTs("","","", seawateramm);
    }

    function test_safetranser() public {
        // This transfer will fail due to the incorrect check in the OwnershipNFTs contract
        ownershipnft.safeTransferFrom(address(this), address(mockcontract), 1);
    }
}

contract MockContract {
    function onERC721Received(address, address, uint256, bytes memory) pure external returns(bytes4) {
        // MockContract correctly implements the function selector
        return IERC721TokenReceiver.onERC721Received.selector;
    }
}

2. SeawaterAMM.sol

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

import {ISeawaterAMM} from "./ISeawaterAMM.sol";

contract SeawaterAMM is ISeawaterAMM {
    address mockSender;

    constructor(address sender) {
        mockSender = sender;
    }

    function positionOwnerD7878480(uint256) view external returns (address) {
        return mockSender;
    }

    function transferPositionEEC7A3CD(uint256 id, address from, address to) external {}

    function positionBalance4F32C7DB(address user) external returns (uint256) {}
}

3. ISeawaterAMM.sol

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

interface ISeawaterAMM {
    function positionOwnerD7878480(uint256 id) external returns (address);

    function transferPositionEEC7A3CD(uint256 id, address from, address to) external;

    function positionBalance4F32C7DB(address user) external returns (uint256);
}

4. IERC721TokenReceiver.sol

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

interface IERC721TokenReceiver {
    function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes calldata _data) external returns (bytes4);
}

Detailed Explanation:

1. Test Setup:

   • A mock contract (MockContract) is created to simulate a valid contract capable of receiving NFTs.
   • The OwnershipNFTs contract is deployed, which is the contract under test, containing the erroneous safeTransferFrom function.

2. Test Execution:

   • In the test_safetranser() function, we attempt to transfer an NFT to the mock contract (MockContract) using the safeTransferFrom() function.
   • Since MockContract correctly implements the onERC721Received function and returns the expected function selector (IERC721TokenReceiver.onERC721Received.selector), the transfer should succeed.

3. Error Encountered:

   • Despite the correct implementation, the transfer fails due to the following incorrect validation in the _onTransferReceived function:

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

   • The check is incorrect because it uses != (not equal), meaning that even when the correct function selector is returned, the transfer is rejected, causing the transaction to revert with the message "bad nft transfer received data".

4. Impact:

   • This error results in valid NFT transfers being rejected (DoS), while potentially incorrect transfers (i.e., to contracts that return arbitrary values or incorrect selectors) could succeed, leading to security and functionality risks.

Test Result

This test fails due to incorrect validation in the safeTransferFrom function. Even though the mock contract returns the correct function selector, the transfer reverts, blocking the valid transfer.

  [18881] testNft::test_safetranser()
    ├─ [11555] OwnershipNFTs::safeTransferFrom(testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], MockContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
    │   ├─ [2435] SeawaterAMM::positionOwnerD7878480(1) [staticcall]
    │   │   └─ ← [Return] testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
    │   ├─ [418] SeawaterAMM::transferPositionEEC7A3CD(1, testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], MockContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
    │   │   └─ ← [Stop] 
    │   ├─ [815] MockContract::onERC721Received(testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1, 0x)
    │   │   └─ ← [Return] 0x150b7a02
    │   └─ ← [Revert] revert: bad nft transfer received data
    └─ ← [Revert] revert: bad nft transfer received data

Ran 1 test suite in 1.40s (1.31ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

This result highlights the Denial of Service (DoS) issue, with the safeTransferFrom function reverting due to faulty logic.

Tools Used

• Foundry: A fast, modular testing framework for smart contracts.
• Solidity: Used to write the PoC and simulate the NFT transfer process.
• VS Code: IDE for writing and testing Solidity contracts.

Recommended Mitigation Steps

To resolve this issue, the validation logic in the _onTransferReceived function should be corrected. Specifically, the condition checking the return value of onERC721Received should be updated to ensure that the function succeeds only when the correct function selector is returned.

Updated Code

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

    // Correct the validation logic to check for equality
    require(
        data == IERC721TokenReceiver.onERC721Received.selector,
        "bad nft transfer received data"
    );
}

Explanation of the Fix:

• The previous validation used !=, which caused valid transfers to fail.
• The fix ensures that the validation uses ==, so the function only reverts if the return value is not the expected function selector (IERC721TokenReceiver.onERC721Received.selector).
• This change ensures that only contracts that correctly implement the onERC721Received function can receive NFTs, preventing denial of service attacks for valid contracts and improper transfers to incorrect ones.

Additional Considerations:

• Test Coverage: After implementing this fix, it is essential to update and expand the test cases to ensure transfers succeed when the recipient contract implements the correct function and fail for contracts with incorrect or missing implementations.
• Gas Optimization: While the current issue is critical, consider optimizing gas usage in future reviews for further improvements to the contract.

Conclusion

This bug in the safeTransferFrom function is critical as it prevents valid transfers and allows potential transfers to contracts without proper NFT handling. The proposed fix resolves this issue by correcting the validation logic, ensuring that only contracts with the correct implementation can receive NFTs. It is strongly recommended to implement this fix to prevent further denial of service or security vulnerabilities in the system.

Assessed type

DoS

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as satisfactory