Lack of support for fee-on-transfer and rebasing ERC20 token

[code423n4]

Lines of code

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

Impact

Lack of support for fee-on-transfer and rebasing ERC20 token

Proof of Concept

.In the current implementation, the code assume the transfered amount matches the receiving amount

quest-protocol\contracts\Erc20Quest.sol:
   66      function _transferRewards(uint256 amount_) internal override {
   67:         IERC20(rewardToken).safeTransfer(msg.sender, amount_);
   68      }

   85          uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
   86:         IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
   87      }

  102      function withdrawFee() public onlyAdminWithdrawAfterEnd {
  103:         IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
  104      }

However, according to

https://github.com/d-xo/weird-erc20#fee-on-transfer

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).

If the ERC20 charge transfer fee, the user cannot claim the fully amount of reward they are entitled.

According to

https://github.com/d-xo/weird-erc20#balance-modifications-outside-of-transfers-rebasing--airdrops

Some tokens may make arbitrary balance modifications outside of transfers (e.g. Ampleforth style rebasing tokens, Compound style airdrops of governance tokens, mintable / burnable tokens).

Because the smart contract does not have method to handle the rebased ERC20 token or different airdropped ERC20 token, which can lock a part of token in the smart contract.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol make it explicit in the doc that the fee-on-transfer and rebasing ERC20 token are not supported.

[c4-judge]

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

[GarrettJMU]

We aren't going to support rebasing erc20. Will leverage the token whitelist in factory to prevent

[c4-sponsor]

GarrettJMU 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 satisfactory

[c4-judge]

kirk-baird marked the issue as duplicate of #630

[c4-judge]

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