code423n4 commented 2 years ago

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.

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.

Prevention: 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:

enum FeeOnTransferStatuses{ NOT_INITIALIZED, HAS_FEE_ON_TRANSFER, DOES_NOT_HAVE_FEE_ON_TRANSFER } 
mapping(IERC20 => FeeOnTransferStatuses) doesThisContractHaveFeeOnTransfer; 
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); 
}

Affected code:

Proof of Concept

Tools Used

Recommended Mitigation Steps

obatirou commented 2 years ago

Fee-on-transfer support (disputed)

Already surfaced (see README)

jack-the-pug commented 2 years ago

Invalid/known-issue per the doc about Deflationary, Rebase and exotic Tokens

code-423n4 / 2022-06-nested-findings

Fee-on-transfer support #54