search

code-423n4 / 2021-05-yield-findings

0 stars 0 forks source link

Vaults are in liquidation forever instead of just for auction length #31

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

The witch can Witch.grab vaults and the vaultOwners[vaultId] field is set to the original owner. The original vault owner is only restored if all debt (balances_.art) is repaid by the liquidation engine.

if (balances_.art - art == 0) { // If there is no debt left, return the vault with the collateral to the owner
    cauldron.give(vaultId, vaultOwners[vaultId]);
    delete vaultOwners[vaultId];
}

Note that there's no check in settle verifying that the auction time (from grab) is not over yet, as well as no check that the vault is actually still undercollateralized.

Impact

Once a vault is grabbed by the witch it'll be susceptible to liquidations forever. All debt has to be repaid to get the vault out of a liquidation state again. An example would be that a vault becomes undercollateralized, the witch grabs it, the network is congested and nobody is able to liquidate it, the auction time is over, the collateral value has increased in the meantime and the vault is not undercollateralized anymore. Liquidators can still liquidate this vault whenever they want which doesn't seem fair to the vault owner.

Recommended Mitigation Steps

Liquidations should only occur during the auction time. If settle is called after auction time (maybe with a small buffer to give liquidators the chance to fully liquidate all collateral at elapsed >= auctionTime), it should restore the original owner and it must be grabbed again by the witch (this also performs a collateralization level check again in grab, which is good).

alcueca commented 3 years ago

Once a vault is grabbed by the witch it'll be susceptible to liquidations forever. All debt has to be repaid to get the vault out of a liquidation state again.

That's the intended behaviour. Once an auction starts, the vault will be liquidated. If the collateral price goes up relative to the debt in the meantime, the user might see more of its collateral back.

The auction time is there to allow us to have several parallel liquidation engines, of which only the Witch has already been implemented. When a liquidation engine grabs a vault but hasn't liquidated it by a set time, other liquidation engines can take the vault from the underperforming liquidation engine.

Once a vault is grabbed by the witch it'll be susceptible to liquidations forever. All debt has to be repaid to get the vault out of a liquidation state again.

We made this choice with v1, and haven't revisited it.