Upgraded Q -> 2 from #169 [1727128613860]

[c4-judge]

Judge has assessed an item in Issue #169 as 2 risk. The relevant finding follows:

2. OwnershipNFTs's approve function allows approved users to also perform approvals.

• Lines of code

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

Impact

OwnershipNFTs's approve function allows approved users to also perform approvals, which can lead to approval loss.

The approve function checks if msg.sender is authorized.

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

This is enforced by the _requireAuthorised which ensures that caller is either owner, owner's operator or approved to spend the token.

    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 is an issue because any user that had be approved to spend the token risks losing their approval after calling the approve function.

Recommended Mitigation Steps

Recommend limiting the approval to the owner and operator instead.

[c4-judge]

alex-ppg marked the issue as duplicate of #65

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid