Unrevoked approvals allow NFT recovery by previous owner

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L98-L107

Vulnerability details

Impact

The vulnerability arises from the fact that after a token transfer, the approval status for the token is not revoked. Specifically, the getApproved[_tokenId] is not updated on transfer.
This allows the previous owner (or any approved address) to reclaim the NFT by using the approval mechanism to re-transfer the token back to themselves. This is critical because the new owner of the NFT may lose their asset without realizing it, leading to potential exploitation, loss of assets, and decreased trust in the platform.

Details

In the provided approve function, any user can approve themselves or another address for a specific token ID:

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

Since the approval is not revoked upon transfer, the previous owner retains the ability to re-transfer the NFT. The _requireAuthorised function is the only check on transfer permission:

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

Step-by-Step PoC:

1. Initial Approval: The owner of a token (owner1) approves an address (addr2) to transfer their token.
2. Token Transfer: The token is transferred from owner1 to newOwner.
3. Approval Not Revoked: The approval for addr2 is not revoked after the transfer.
4. NFT Recovery: addr2 can still use their approval to transfer the NFT back to themselves, effectively recovering the NFT from newOwner.

Note to the judge: there is no existing tests for this specific smart contract (because it is in solidity). A coded POC for this easy-to-understand vulnerability would involve to create all deployment logic.

Tools Used

Manual code review

Recommended Mitigation Steps

To prevent this vulnerability, any existing approvals should be revoked when a token is transferred. This can be achieved by adding a line in the transfer function to clear the approval:

getApproved[_tokenId] = address(0);

This line should be added to the token transfer function to ensure that any previously approved addresses cannot transfer the NFT after it has been sold or transferred to a new owner.

Assessed type

Token-Transfer

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The submission details how approvals are not cleared whenever an NFT transfer occurs, permitting the NFT to be recovered after it has been transferred which breaks a crucial invariant of NFTs and would affect any and all integrations of the NFT (i.e. staking systems, marketplaces, etc.). As such, I believe a high-risk severity rating is appropriate.