Fee-on-transfer ERC20 reward tokens will cause accounting error and cause some users to not get paid

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L45-L54
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87

Vulnerability details

Impact

Fee-on-transfer token will affect accounting. Some users may not trade their receipt for rewards due to underflow

Proof of Concept

When the quest creator deploys the contract, he has to fund the contract with maxTotalRewards() and maxProtocolRewards().

function maxTotalRewards() public view returns (uint256) {
    return totalParticipants * rewardAmountInWeiOrTokenId;
}


/// @notice Function that gets the maximum amount of rewards that can be claimed by the protocol or the quest deployer
/// @dev The 10_000 comes from Basis Points: https://www.investopedia.com/terms/b/basispoint.asp
/// @return The maximum amount of rewards that can be claimed by the protocol or the quest deployer
function maxProtocolReward() public view returns (uint256) {
    return (maxTotalRewards() * questFee) / 10_000;
}

For example, the quest deployer deposits 10_000 USDC worth of FOT tokens, with the transfer tax being 5%. The quest fee is 20%, so 8_000 USDC worth of FOT tokens should be distributed to successful participants and 2_000 USDC worth of FOT tokens should be paid to the deployer. Lets say there are 100 participants and each participant gets 80 USDC worth of FOT tokens for completing the quest.

When quest deployer deposits 10_000 USDC worth of FOT tokens, the contract now has 9_500 USDC worth of FOT tokens because of transfer fee. Of the 9_500 USDC worth, 7_600 USDC is for the users and 1_900 USDC is for the protocol fee. If every participant manages to finish the quest and get a receipt, the payout should be 8_000 USDC worth but there is only 7_600 USDC to be distributed. Some users will not get paid. Either that or the protocol will get less fees than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

Either ban all known fee-on-transfer tokens or make sure to check token value before and after transfer.

[c4-judge]

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

[jonathandiep]

We have a token reward allowlist and we probably won't want to include tokens that have a transfer fee

[c4-sponsor]

jonathandiep marked the issue as sponsor acknowledged

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