Incorrect ERC721 Approval Check Allows Unauthorized Token Transfers

[c4-bot-4]

Lines of code

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

Vulnerability details

Summary

The _requireAuthorised function is a critical security check in the OwnershipNFTs contract, used to validate whether an address is allowed to transfer a specific token.

The authorization check in _requireAuthorised contains a logical error in its implementation of the ERC721 standard's approval mechanism.

Code Snippet

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

function _requireAuthorised(address _from, uint256 _tokenId) internal view {
    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!");
}

The function checks if msg.sender == getApproved[_tokenId], but it should be checking if msg.sender == getApproved[_tokenId] && getApproved[_tokenId] != address(0). The current implementation allows transfers even when getApproved[_tokenId] is set to the zero address, which should not be considered a valid approval.

Impact

This bug could allow unauthorized transfers of tokens in certain scenarios, potentially leading to loss of assets for token owners.

Scenario

1. Alice owns token #123
2. getApproved[123] is set to address(0) (default value, meaning no specific address is approved)
3. Bob calls transferFrom(alice, bob, 123)
4. The transfer succeeds incorrectly because msg.sender (Bob) == getApproved[123] (address(0))

Fix

Modify the _requireAuthorised function to correctly check for a non-zero approved address:

function _requireAuthorised(address _from, uint256 _tokenId) internal view {
    bool isAllowed =
        msg.sender == _from ||
        isApprovedForAll[_from][msg.sender] ||
        (msg.sender == getApproved[_tokenId] && getApproved[_tokenId] != address(0));

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

Assessed type

Token-Transfer