Yield Witch v2 contest - wastewa's results

Lines of code

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L181-L183

Vulnerability details

Impact

Attacker can create a vault that can never be liquidated, potentially allowing them to keep their loan for as long they desire and distort collateral-to-token balance. This will also distort the efficiency of the Witch contract and liquidation mechanism.

Proof of Concept

Attack just needs to:

1. Build a vault
2. Take out loan
3. Use give function on the Ladle.sol contract, with the receiver address set to the address of the Witch.sol contract (or any other witch in otherWitches variable).

After this no liquidator will able to call the auction function on that vault, even if the collateral is worth less than the loan.

This is because this if-statement (show below) will throw the error VaultAlreadyUnderAuction because the vault owner will be equal to the contract of the Witch contract (or any other witch in otherWitches variable):

function auction(bytes12 vaultId, address to)
        external
        returns (DataTypes.Auction memory auction_)
    {
        DataTypes.Vault memory vault = cauldron.vaults(vaultId);
        if (vault.owner == address(this) || otherWitches[vault.owner]) {
            revert VaultAlreadyUnderAuction(vaultId, vault.owner);
        }

Tools Used To Recreate

Forge tests inside repo

Recommended Mitigation Steps

Assuming we only want to change code within the Witch contract, we should aim to change the check condition to rely on something other than ownership of vault (like start time of auction, as was done in the older Witch contract).

Old code in old Witch.sol#L91 contract: require (auctions[vaultId].start == 0, "Vault already under auction");

We need to repeat this check for otherWitches as well if we want to keep this in the code. So I recommend adding a function to all Witches (keep this in some shared interface) that returns whether or not an auction is currently running under that Witch, and then doing a simple for-loop checking all Witches for that vault auction.

This fix will allow any Vaults that are supposed to be liquidated to be liquidated, regardless of whether a user has given the vault away to a Witch.