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

joestakey - claimer not supporting ERC-721 tokens can be DOS #536

Closed
github-actions bot opened this issue Feb 22, 2023 · 2 comments
Closed

joestakey - claimer not supporting ERC-721 tokens can be DOS #536

github-actions bot opened this issue Feb 22, 2023 · 2 comments
Labels
Low/Info A valid Low/Informational severity issue Non-Reward This issue will not receive a payout Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@github-actions
Copy link

joestakey

medium

claimer not supporting ERC-721 tokens can be DOS

Summary

Anyone can prevent a claimer not supporting ERC-721 from claiming.

Vulnerability Detail

_claimAtomicBounty loops through the ERC20 token deposits, then through the NFT deposits to send the reward

File: contracts/ClaimManager/Implementations/ClaimManagerV1.sol
150:         for (uint256 i = 0; i < _bounty.getNftDeposits().length; i++) {
151:             _bounty.claimNft(_closer, _bounty.nftDeposits(i)); 

This transfers the NFT to the claimer using a safeTransferFrom call.

File: contracts/Bounty/Implementations/BountyCore.sol
257:     function _transferNft(
258:         address _tokenAddress,
259:         address _payoutAddress,
260:         uint256 _tokenId
261:     ) internal virtual {
262:         IERC721Upgradeable nft = IERC721Upgradeable(_tokenAddress);
263:         nft.safeTransferFrom(address(this), _payoutAddress, _tokenId);
264:     }

The issue is that the safeTransferFrom call reverts if the _payoutAddress does not support ERC721 tokens.

It means a claimer call can be DOS by front-running them with a fundBountyNFT() call, adding a NFT reward to the bounty, which will make the claimer unable to claim.

Impact

Valid claimer is unable to claim their ERC20 token rewards.
It is likely that bounty issuers will specify the token the reward will be in.
Claimer could be using a smart wallet that does not support ERC-721 token, and seeing a bounty paying in MATIC (or any ERC20 token), would participate in the bounty. In such case, the attack would prevent them from claiming.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L150-L151
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L262-L263

Tool used

Manual Review

Recommendation

Consider using a try/catch approach for claiming.

@github-actions github-actions bot added the Medium A valid Medium severity issue label Feb 22, 2023
@FlacoJones FlacoJones added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Feb 22, 2023
@FlacoJones
Copy link

@hrishibhat
Copy link
Contributor

Considering this issue a low as this affects only those who set to contract the do not receive ERC721.

@sherlock-admin sherlock-admin added Low/Info A valid Low/Informational severity issue Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue labels Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Low/Info A valid Low/Informational severity issue Non-Reward This issue will not receive a payout Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

3 participants