Fee-on-transfer support

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L423 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L429

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

1. Contract calls transfer from contractA 100 tokens to current contract
2. Current contract thinks it received 100 tokens
3. It updates internal balance sheet +100 tokens
4. While actually contract received only 90 tokens
5. That breaks whole math for given token
6. 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.

1. Check how much contract balance is increased/decreased after every transfer (costs more gas)
2. 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:

1. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L423
2. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L429

Proof of Concept

Tools Used

Recommended Mitigation Steps

---

[GalloDaSballo]

This looks like a generic copy paste, completely disagree.

Funnily enough the linked code actually would work fine with feeOnTransfer tokens