Neglecting the transfer fees from some ERC20 tokens
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). The STA transfer fee was used to drain $500k from several balancer pools.
Vulnerability description
Should a fee-on-transfer token be added to the PublicVault, the tokens will be locked in the PublicVault.sol contract. Depositors will be unable to withdraw their rewards. In the current implementation, it is assumed that the received amount is the same as the transfer amount. However, due to how fee-on-transfer tokens work, much less will be received than what was transferred. As a result, later users may not be able to successfully withdraw their shares, as it may revert in function _redeemFutureEpoch() part of code:
WithdrawProxy(s.epochData[epoch].withdrawProxy).mint(shares, receiver);
when WithdrawProxy is called due to insufficient balance.
Proof of Concept
Fee-on-transfer scenario:
1. Contract calls transfer from contractA 100 tokens to current contract
2. Current contract thinks it received 100 tokens
3. It updates balances to increase +100 tokens
4. While actually contract received only 90 tokens
5. That breaks whole math for given token
Mitigation
There are two possibilities:
- Consider comparing the before and after balances to get the actual transferred amount.
- Alternatively, disallow tokens with fee-on-transfer mechanics to be added as tokens.