Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

approve() function in the OwnershipNFT contract has incorrect authorization check #130

Closed
howlbot-integration bot opened this issue Sep 16, 2024 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-65 edited-by-warden 🤖_10_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

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

Vulnerability details

Impact

The current functionality of OwnershipNFTs smart contract allows to make approvals of the NFT positions to other addresses. The problem is that the contract is supposed to be ERC-721 compliant but its implementation of authorization checks differs from the one realized in the ERC-721 contract itself.

Proof of Concept

Here's the current approve() behavior:

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

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

It calls _requireAuthorised() function as well as the functions for NFT transfer do for authorization purposes. But, in comparison with ERC-721 standard, it uses different implementation:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L415-417

// We do not use _isAuthorized because single-token approvals should not be able to call approve

            if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) {
                revert ERC721InvalidApprover(auth);
            }

As you can see, there are several deviations from the ERC-721 standard:

  1. The function requireAuthorized() requires for the from address (that is a msg.sender who calls approve()) to be the owner while in the ERC-721 standard it's auth address.

  2. It checks whether msg.sender is in the getApproved mapping for the given tokenID while in the ERC-721 standard it's only isApprovedForAll().

  3. isApprovedForAll() checks if the isApprovedForAll[_from][msg.sender] statement is true (where _from is always msg.sender) where in the implementation of the standard it checks for the isApprovedForAll(owner, auth) where auth can differ from the owner.

This makes the contract non ERC-721 compatible and leads to the users not being able to give approvals of their tokenIDs to other addresses as it's required

Tools Used

Manual review.

Recommended Mitigation Steps

Change the check for authorization inside of the approve() function to make it ERC-721 compatible.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_10_group AI based duplicate group recommendation bug Something isn't working duplicate-65 edited-by-warden sufficient quality report This report is of sufficient quality labels Sep 16, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Sep 23, 2024
@rodiontr
Copy link

@alex-ppg I think this issue indicates the problem as you described in #65:

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.

here:

auth basically implies operators as well

The function requireAuthorized() requires for the from address (that is a msg.sender who calls approve()) to be the owner while in the ERC-721 standard it's auth address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-65 edited-by-warden 🤖_10_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants