Incompatibility with Rebasing or Deflationary or Inflationary Tokens

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L67
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L86
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L103

Vulnerability details

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).
Though the Rabbithole protocol first checks if the ERC20 token is added to rewardAllowlist however it is very likely that fee on transfer tokens get added. USDT/USDC are very popular options to be added and in future when they start charging fee on transfer, the rabbithole protocol will behave unexpectedly at several points.

The Rabbithole protocol does not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L67
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L86
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L103

Recommended Mitigation Steps

• Ensure that to check previous balance/after balance equals to amount for any rebasing/inflation/deflation
• Add support in contracts for such tokens before accepting user-supplied tokens
• Consider supporting deflationary / rebasing / etc tokens by extra checking the balances before/after or strictly inform your users not to use such tokens if they don’t want to lose them.

Example code to mitigate :

uint256 balanceBefore = token_.balanceOf(address(this));
IERC20(token).safeTransferFrom(_from, address(this), _amount);
uint256 balanceAfter = token_.balanceOf(address(this));

if (balanceAfter.sub(balanceBefore) != amount_) {
// the token is deflationary, we don't support that.
revert("Deflationary tokens are not supported!");
}

[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 primary issue

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked issue #454 as primary and marked this issue as a duplicate of 454

[c4-judge]

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