ERC20 can have tax or be deflanatory

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L179
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L86
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L66
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96

Vulnerability details

Vulnerability details

A transfer-on-fee token or a deflationary/rebasing token, causing the received amount to be less than the accounted amount. For instance, a deflationary tokens might charge a certain fee for every transfer() or transferFrom()

There are two scenarios. Both of them stem from the fact that RH Quests support ANY kind of ERC20 tokens, hence it is possible that such that such a transfer-on-fee token or a deflationary/rebasing token be used in the protocol.

The first scenario is when a quest has ended, withdrawRemainingTokens is supposed to be called before the withdrawFee() function as shown in the diagram.

The formula in withdrawRemainingTokens is uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens; ,which translates to "everything, but the protocolFee()".

Then, if an ERC20 token that has a certain fee for transfer is used, the he amount available in the contract will be less than what the protocolFee() function is expecting.

As a result, the protocolFeeRecipient would never be able to call withdrawFee(), because there won't be enough tokens in balance.

The second scenario involves the claiming of token rewards through Quest.sol. Every claim initiated by a user triggers the transferRewards function. Depending on the amount of participants, even if the fee is not that high there's a point where its technically possible for the protocol to be out of balance.

As a result, all the claim requests will fail.

POC

Scenario 1

1. Create quest
2. Quest starts
3. Quest ends
4. Quest is not fully participated
5. Protocol withdraws the remaining tokens through withdrawRemainingTokens()
6. withdrawFee() reverts due to insufficient balance

Scenario 2

1. Create quest
2. Quest starts
3. Multiple users claiming rewards, but there's a tax on each transfer.
4. Users are no longer able to claim any rewards even though they are eligible.

Tools

Manual testing

Recommended Mitigation Steps

Make an allowlist for only specific tokens that you are aware will be working well with the logic imposed by the protocol.

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-sponsor]

waynehoover marked the issue as sponsor acknowledged

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

kirk-baird marked the issue as duplicate of #630

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)