Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

cergyk - A claiming contract not implementing ERC721Receiver can be made to revert by an NFT funder #132

Closed
github-actions bot opened this issue Feb 21, 2023 · 4 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout

Comments

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

cergyk

medium

A claiming contract not implementing ERC721Receiver can be made to revert by an NFT funder

Summary

On TieredFixedBounty and TieredPercentageBounty, any user can associate a whitelisted NFT reward with the paymentToken reward. Unfortunately this may revert the whole payment if the receiving address is a contract but not an ERC721Receiver.

Vulnerability Detail

If we take the simpler example of an atomic bounty:
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L123-L166

A malicious user could detect that the closer is trying to claim the bounty with a contract which is not an ERC721Receiver and block his payment by funding the bounty with an NFT.

Impact

Bounty admin has to take control and either:

  • change the winner (tiered bounties)
  • call claim again with a different closer for AtomicBounty

Code Snippet

Tool used

Manual Review

Recommendation

Do not fail on transfer of NFTs (use try/catch), eventually the nft depositor can have his NFT refunded.

Please note that this scenario is different from #1, since in this case the nft stays in the contract. So the grieving described here should be possible even if #1 is fixed and nftDeposits is updated correctly.

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue labels Feb 21, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Mar 7, 2023
@pauliax
Copy link

pauliax commented Mar 8, 2023

Escalate for 31 USDC. This is not a duplicate of #261 because Ongoing bounty does not distribute NFT rewards, so ERC721Receiver is not triggered on the payout address. This issue talks about safely transferring NFT to the payout address:

    function _transferNft(
        address _tokenAddress,
        address _payoutAddress,
        uint256 _tokenId
    ) internal virtual {
        IERC721Upgradeable nft = IERC721Upgradeable(_tokenAddress);
        nft.safeTransferFrom(address(this), _payoutAddress, _tokenId);
    }

safeTransferFrom requires the recipient to implement the ERC721Receiver if it is not an EOA.

Anyway, I think this issue should be low because if this situation happens, admins can set a new payout address which is EOA.

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 8, 2023

Escalate for 31 USDC. This is not a duplicate of #261 because Ongoing bounty does not distribute NFT rewards, so ERC721Receiver is not triggered on the payout address. This issue talks about safely transferring NFT to the payout address:

    function _transferNft(
        address _tokenAddress,
        address _payoutAddress,
        uint256 _tokenId
    ) internal virtual {
        IERC721Upgradeable nft = IERC721Upgradeable(_tokenAddress);
        nft.safeTransferFrom(address(this), _payoutAddress, _tokenId);
    }

safeTransferFrom requires the recipient to implement the ERC721Receiver if it is not an EOA.

Anyway, I think this issue should be low because if this situation happens, admins can set a new payout address which is EOA.

You've created a valid escalation for 31 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Mar 8, 2023
@hrishibhat
Copy link
Contributor

Escalation accepted

Not a duplicate of #261 and a low issue

@sherlock-admin
Copy link
Contributor

Escalation accepted

Not a duplicate of #261 and a low issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout and removed Escalated This issue contains a pending escalation Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Reward A payout will be made for this issue labels Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

3 participants