Platform: Code4rena
Start Date: 14/07/2022
Pot Size: $25,000 USDC
Total HM: 2
Participants: 63
Period: 3 days
Judge: PierrickGT
Total Solo HM: 1
Id: 147
League: ETH
Rank: 3/63
Findings: 2
Award: $5,073.70
🌟 Selected for report: 0
🚀 Solo Findings: 0
Vault cannot be liquidated
auction can be called with any address as the 'to' address. A majority of ERC20 tokens will revert if a transfer is initiated to address(0), notably, including USDC. Since the auctioneer is paid each time a payment is made towards the vault, every payment would fail because it would revert when sending the auctioneer's portion to address(0). This results in the vault being impossible to liquidate.
Add this line to the start of auction:
require(to != address(0))
#0 - ecmendenhall
2022-07-19T03:05:37Z
Agree with this one—for Yield specifically, this is an issue for both USDC and FRAX.
#1 - alcueca
2022-07-21T10:23:23Z
Duplicate of #116
#2 - PierrickGT
2022-08-03T16:15:26Z
Most critical vulnerability found during the audit but since the description of the attack is less detailed than the one provided in #116, I have labelled it as a duplicate.
🌟 Selected for report: hickuphh3
Also found by: 0x29A, 0x52, 0xNazgul, Chom, Deivitto, ElKu, Funen, IllIllI, Meera, ReyAdmirado, SooYa, TomJ, Trumpero, Waze, __141345__, ak1, asutorufos, c3phas, cRat1st0s, csanuragjain, delfin454000, exd0tpy, fatherOfBlocks, hake, hansfriese, horsefacts, hyh, karanctf, kenzo, kyteg, ladboy233, pashov, peritoflores, rajatbeladiya, rbserver, reassor, rokinot, simon135, wastewa
40.8106 USDC - $40.81
https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L286-L333 https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L344-L383
Unfair liquidation of collateralized vault
Providing a reward to users that start auction incentivizes users to create auctions for undercollateralized vaults but there is no incentive to cancel auctions for user who's vault move back above this threshold, creating an unfair dynamic for the vault owners. With how volatile these markets are a vault close to liquidation could go above and below the threshold multiple times a day. On volatile days, a user could use a bot to monitor prices and call liquidation during lows and then if prices rebound, vaults would be bought out by liquidators even though they might not be undercollateralized anymore. Vault owners would have to constantly monitor and call cancel. The average user isn't going to use a bot which puts them at an unfair disadvantage.
To avoid this the following line should be added to payBase and payFYToken:
require(cauldron.level(vaultId) < 0, "Not undercollateralized")
#0 - HickupHH3
2022-07-18T14:39:52Z
dup of #91
#1 - ecmendenhall
2022-07-19T03:18:19Z
(Adding 👀 to some of these to indicate duplicates)
#2 - alcueca
2022-07-21T10:39:02Z
This finding digs a bit into morals and system design, more than finding a bug.
In our system, a user gets liquidated if their position becomes undercollateralized, that's it. At the block where they are undercollateralized we get rid of the vault.
Could we be nicer to the users and check if we can save them in a later block? Yes, we could, but at a cost. Liquidating vaults would be more expensive (due to the additional collateralization check in each payment function) and that cost would be borne by someone, not necessarily the liquidator.
During design, we preferred the simpler approach. It could have been documented, true.
As a courtesy to users, we added the cancel function, which we don't think will ever be used.
I do not think this is an issue, but a design decision. No funds are lost, since both vault owners and liquidators are our users. Liquidators are not attackers.
#3 - PierrickGT
2022-07-28T15:22:54Z
Agree with the sponsor, the scenario outlined by the warden is plausible and the system would behave as intended. Since this issue is related to a design decision, I will downgrade it to a QA Report.