0x52 - Adversary can brick bounty payouts by calling fundBountyToken but funding it with an ERC721 token instead

[github-actions]

0x52

high

Adversary can brick bounty payouts by calling fundBountyToken but funding it with an ERC721 token instead

Summary

recieveFunds is only meant to be called with an ERC20 token but _receiveERC20 is generic enough to work with an ERC721 token. An adversary could call this with an ERC721 token to add it as a bounty reward. The problem is that the payout functions would completely break when trying to send it as a payout. The result is that afterwards the bounty would be completely broken.

Vulnerability Detail

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L197-L209

BountyCore#_receiveERC20 makes two calls to the underlying token contract. The first is the transferFrom method which exists functions identical to the ERC20 variant using token id instead of amount.

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L291-L299

The other call that's made is balanceOf which is also present in ERC721

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L221-L228

On the flipside, when withdrawing and ERC20 it uses the transfer method which doesn't exist in ERC721. The result is that ERC721 tokens can be deposited as ERC20 tokens but can't be withdrawn. Since the contract will revert when trying to payout the ER721 token all payouts from the bounty will no longer work.

Submitting as high risk because when combined with refund locking methods it will result in all deposited tokens being stuck forever.

Impact

Bounty will be permanently unclaimable

Code Snippet

Tool used

Manual Review

Recommendation

Split the whitelist into an NFT whitelist and an ERC20 whitelist, to prevent a whitelisted NFT being deposited as an ERC20 token.

[FlacoJones]

Will fix with an explicity OpenQTokenWhitelist, no arbitrary token addresses

[FlacoJones]

OpenQDev/OpenQ-Contracts#113 and OpenQDev/OpenQ-Contracts#116 and OpenQDev/OpenQ-Contracts#114 effectively remove the possibility of this exploit

[pauliax]

Escalate for 52 USDC.
I think this issue is essentially the same as #62. It does not matter if the token is ERC20, ERC721, ERC1155 or ERC777, or whatever, what matters is that this impostor contract pretends to comply with the funding function only later to reveal its dark side. Based on my interpretation, all the issues that are talking about feeding the bounty with 'wrong' tokens should be grouped together under one issue, because all of them are giving basically the same effect.

[sherlock-admin]

Escalate for 52 USDC.
I think this issue is essentially the same as #62. It does not matter if the token is ERC20, ERC721, ERC1155 or ERC777, or whatever, what matters is that this impostor contract pretends to comply with the funding function only later to reveal its dark side. Based on my interpretation, all the issues that are talking about feeding the bounty with 'wrong' tokens should be grouped together under one issue, because all of them are giving basically the same effect.

You've created a valid escalation for 52 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.

[Evert0x]

Escalation rejected. OpenQ used the same method of locking down everything to a whitelist as a means to fix all of them but they are not all the same and would have needed different fixes for each one if OpenQ had wanted to keep the funding process open.

[sherlock-admin]

Escalation rejected. OpenQ used the same method of locking down everything to a whitelist as a means to fix all of them but they are not all the same and would have needed different fixes for each one if OpenQ had wanted to keep the funding process open.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[jacksanford1]

Lead Senior Watson comment on PR #113:

Changes look good.

Token requirement has been simplified to just a whitelist. It makes the rest changes to support this revision. tokenAddressLimitReach method has been removed from DepositManagerV1. _tokenAddressLimit has been removed from TokenWhitelist constructor. TOKEN_ADDRESS_LIMIT has been removed along with it's setter. Tests have been updated to accommodate changes.

Lead Senior Watson comment on PR #116:

Changes look good. Requires that funder is the issuer. This prevents a whole host of potential exploits in exchange for closing the otherwise open funding model.

Lead Senior Watson comment on PR #114:

Changes look good. Removes all logic from bounties. Seems to have gotten all related code. Will keep my out for any that was missed on when checking the rest of the PRs