Platform: Code4rena
Start Date: 06/01/2022
Pot Size: $60,000 USDC
Total HM: 20
Participants: 33
Period: 7 days
Judge: LSDan
Total Solo HM: 9
Id: 67
League: ETH
Rank: 19/33
Findings: 2
Award: $412.91
🌟 Selected for report: 1
🚀 Solo Findings: 0
Ruhum
When swapping tokens you should include a slippage check. Meaning, if a swap didn't result in a minimum amount of tokens the transaction reverts. The NonUSTStrategy contract uses Curve to swap the underlying token to UST. There, the contract doesn't use any slippage checks and leaves the minimum amount at 0. That makes the contract vulnerable to sandwich attacks.
none
add a reasonable minimum amount or check the amount received and verify that it's an expected amount
#0 - naps62
2022-01-11T18:41:40Z
duplicate of #8
#1 - dmvt
2022-01-27T11:47:44Z
duplicate of #7
🌟 Selected for report: Ruhum
Also found by: Tomio, WatchPug, harleythedog
322.8543 USDC - $322.85
Ruhum
Some ERC20 tokens charge a fee for every transfer. If the underlying token of a vault is such a token any deposit to the protocol will fail.
Some tokens have the possibility of adding fees later on, e.g. USDT. So those have to be covered too.
Generally, the user would also receive fewer tokens on withdrawing in such a scenario but that's not the protocol's fault.
I rated the issue as medium since part of the protocol become unavailable in such a situation.
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L583-L585
_transferAndCheckUnderlying() is used to deposit and sponsor the vault. It checks that after a safeTransferFrom() the same exact amount is sent to the balance of the vault. But, if fees are enabled the values won't match, causing the function to revert. Thus, it won't be able to deposit or sponsor the vault in any way.
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L162
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L266
none
One possibility would be to simply not use ERC20 tokens with fees.
#0 - r2moon
2022-01-11T15:39:46Z
We don't use tokens with fees
#1 - naps62
2022-01-11T18:28:58Z
The only place where we mention USDT is on an old pitch deck (not up to date anymore). The codebase itself doesn't mention it, and all tests are done with USDC and DAI as examples
#2 - dmvt
2022-01-27T22:22:59Z
I'm going to let this issue stand given that #164 is also valid. Supported or not, fee on transfer tokens would cause a loss of funds in the scenario described. As the USDT example shows (in both issues), many stables can be upgraded and add a fee later.
#3 - naps62
2022-02-21T13:27:17Z
There's no action point here, as the recommended steps already match what we're doing:
One possibility would be to simply not use ERC20 tokens with fees.
Therefore closing this