Fee-on-transfer tokens support must be forbidden or allowed with amount update #18

code423n4 · 2022-05-12T12:11:01Z

Lines of code

Vulnerability details

Impact

Every time transferFrom or transfer function in ERC20 standard is called there is a possibility that underlying smart contract did not transfer the exact amount entered.
It is required to find out contract balance increase/decrease after the transfer to allow fee-on-transfer tokens or forbid non-standard tokens.
This pattern also prevents from re-entrancy attack vector.

Proof of Concept

POC (re-entrancy for fee-on-transfer tokens):

Contract calls transfer from contractA 100 tokens to current contract
Current contract thinks it received 100 tokens
It updates internal balance sheet +100 tokens
While actually contract received only 90 tokens
That breaks whole math for given token
Now imagine some fake token which does not send anything
Attacker could run re-entrancy and boosting deposits in the storage while transferring nothing.
At the end drain tokenOut (if it is DEX) or withdraw something else based on large deposit.

Tools Used

Recommended Mitigation Steps

There are several possible scenarios to prevent that.

Check how much contract balance is increased/decreased after every transfer (costs more gas)
Make a separate contract that checks if the token has fee-on-transfer and store bool value depending on the result.
If there is fee-on-transfer you can throw a require not allowing to use such token in the system while still saving lots of gas checking it only once.
Or if you still want to allow fee-on-transfer tokens, amount variable must be updated to the balance difference after and before transfer.

Recommended example code:

mapping(IERC20 => bool) doesThisContractHaveFeeOnTransfer;
enum FeeOnTransferStatuses{ NOT_INITIALIZED, HAS_FEE_ON_TRANSFER, DOES_NOT_HAVE_FEE_ON_TRANSFER }
error FeeOnTransferTokensAreForbidden();
...
function deposit(IERC20 token, address from, uint256 amount) public {

    // reverting for fee-on-transfer tokens
    if (doesThisContractHaveFeeOnTransfer[token] == FeeOnTransferStatuses.HAS_FEE_ON_TRANSFER) {
        revert FeeOnTransferTokensAreForbidden();
    }

    // NOT_INITIALIZED is the default value == 0
    if (doesThisContractHaveFeeOnTransfer[token] == FeeOnTransferStatuses.NOT_INITIALIZED) {
        uint256 balanceBefore = token.balanceOf(address(this)); // remembering asset balance before the transfer
        token.safeTransferFrom(from, address(this), amount);
        uint256 balanceAfter = token.balanceOf(address(this));

        // making sure exactly amount was transferred
        if (balanceAfter != balanceBefore + amount) {            
            revert FeeOnTransferTokensAreForbidden();
        }

        // IMPORTANT! if you allow fee-on-transfer tokens make sure to update amount with the actual balance increase/decrease
        // or make sure balanceAfter - balanceBefore == amount using require
        // amount = balanceAfter - balanceBefore; // commented because we skip fee-on-transfer tokens above

        doesThisContractHaveFeeOnTransfer[token] = FeeOnTransferStatuses.DOES_NOT_HAVE_FEE_ON_TRANSFER; // making sure to not enter this if clause anymore for given token
    } else {
        // token does not have fee-on-transfer here
        token.safeTransferFrom(from, address(this), amount);
    }

    // no re-entrancy vector attack below here 
    // amount is set to exactly how much contract balance was increased

    registerDeposit(from, amount); 
}

0xMaharishi · 2022-05-25T16:01:57Z

Tokens used in Aura do not have transfer fees

dmvt · 2022-06-20T16:38:39Z

There are several cases in the code reported where the token in question comes from an external (non-admin, non-protocol) source. One of these is the addReward functionality (ExtraRewards). This would indeed cause an accounting issue and allow a potential malicious actor to send rewards which cause distribution to fail due to lack of funds. Just because you don't plan to use fee on transfer tokens, does not mean they will not be used. This should be protected against in the scenarios where it could cause an issue.

That said, this clearly requires external factors and relies on hypothetical attack motivation that seems unlikely to me. Additionally, the warden just threw every transfer line into the report, making this report very hard to parse. I think it should be included as a medium risk, but most of the lines above are actually invalid.

dmvt · 2022-07-08T17:50:50Z

Duplicate of #176

code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels May 12, 2022

code423n4 added a commit that referenced this issue May 12, 2022

MaratCerby issue #18

4641caf

0xMaharishi added invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels May 25, 2022

dmvt added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed invalid This doesn't seem right 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jun 20, 2022

dmvt mentioned this issue Jun 20, 2022

Users may lose rewards to other users if rewards are given as fee-on-transfer tokens #176

Open

dmvt marked this as a duplicate of #176 Jul 8, 2022

dmvt closed this as completed Jul 8, 2022

dmvt added the duplicate This issue or pull request already exists label Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee-on-transfer tokens support must be forbidden or allowed with amount update #18

Fee-on-transfer tokens support must be forbidden or allowed with amount update #18

code423n4 commented May 12, 2022

0xMaharishi commented May 25, 2022

dmvt commented Jun 20, 2022 •

edited

Loading

dmvt commented Jul 8, 2022

Fee-on-transfer tokens support must be forbidden or allowed with amount update #18

Fee-on-transfer tokens support must be forbidden or allowed with amount update #18

Comments

code423n4 commented May 12, 2022

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

0xMaharishi commented May 25, 2022

dmvt commented Jun 20, 2022 • edited Loading

dmvt commented Jul 8, 2022

dmvt commented Jun 20, 2022 •

edited

Loading