approve() shouldn't be called by the approved address for that tokenId

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The current implementation of OwnershipNFTs::approve() function allows the approved address to call approve() on the same tokenId for which he is approved. This puts the owner at risk as any approved address can proceed to approve any other users to this token without his consent.

Proof of Concept

EIP721 states that approve():

    ///  Throws unless `msg.sender` is the current NFT owner, or an authorized operator of the current owner.

However, the current implementation breaks this access control provided by the standard.

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

In the contract, approve() uses _requireAuthorised() which checks if:

• The caller is owner
• caller is approved for that tokenId
• caller is an operator

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

This setup violates the set standard as it should only check the caller is current NFT owner, or an authorized operator.

Tools Used

Manual Review

Recommended Mitigation Steps

When approve()is called, check only if msg.sender is the owner, or isApprovedForAll.

    function approve(address _approved, uint256 _tokenId) external payable {
-       _requireAuthorised(msg.sender, _tokenId);
+       owner = ownerOf(_tokenId);
+       require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "Caller not authorized");
        getApproved[_tokenId] = _approved;
    }

Assessed type

Access Control

[af-afk]

We feel that this is perhaps better considered as medium, in that approving someone access to the token enables them to transfer it on the owner's behalf anyway, so the risk profile is the same.

[alex-ppg]

The Warden's submission states that an approved user can set yet another approval which is incorrect.

The code presently permits only the owner to perform an approval as it mandates that the _from argument of the OwnershipNFTs::_requireAuthorised function is equal to the owner of the NFT. As such, an approved user may pass the isAllowed check but will fail at the one right after.

The code indeed contains a flaw in the sense that an operator is not able to approve an NFT, however, this submission did not outline this vulnerability and the vulnerability itself is of limited impact.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid