Yield Witch v2 contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L232

Vulnerability details

Impact

It was observed that the debt and collateral which moves for Auction is calculated incorrectly. In case where line.proportion is set to small value, chances are art will become lower than min debt. This causes whole collateral to go for auction, which was not expected

Proof of Concept

1. Assume line.proportion is set to 10% which is a valid value

2. Auction is started on Vault associated with collateral & base representing line from Step 1

3. Now debt and collateral to be sold are calculated in _calcAuction

uint128 art = uint256(balances.art).wmul(line.proportion).u128();
        if (art < debt.min * (10**debt.dec)) art = balances.art;
        uint128 ink = (art == balances.art)
            ? balances.ink
            : uint256(balances.ink).wmul(line.proportion).u128();

4. Now lets say debt (art) on this vault was amount 10, collateral (ink) was amount 9, debt.min * (10**debt.dec) was amount 2

5. Below calculation occurs

uint128 art = uint256(balances.art).wmul(line.proportion).u128(); // which makes art = 10*10% =1
        if (art < debt.min * (10**debt.dec)) art = balances.art;   // since 1<2 so art=10
        uint128 ink = (art == balances.art)                                 // Since art is 10 so ink=9
            ? balances.ink
            : uint256(balances.ink).wmul(line.proportion).u128();

6. So full collateral and full debt are placed for Auction even though only 10% was meant for Auction. Even if it was lower than min debt, auction amount should have only increased upto the point where minimum debt limit is reached

Recommended Mitigation Steps

Revise the calculation like below

uint128 art = uint256(balances.art).wmul(line.proportion).u128();
uint128 ink=0;
        if (art < debt.min * (10**debt.dec)) 
{
art = debt.min * (10**debt.dec);
(balances.ink<art) ? (ink=balances.ink) : (ink=art)
} else {
ink=uint256(balances.ink).wmul(line.proportion).u128();
}

Zero address checks

Contract: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L176

Issue: Check that auctioneer address is not 0. This mean the funds would be lost while paying the auctioneer cut once someone pay the debt

Recommendation: Add below check in auction function

require(to!=address(0), "Invalid address");

Add pause feature if under attack

Contract: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol

Issue: If contract is under attack and Auctioned Vault interaction for buying collateral need to be stopped, then currently there is no way.

Recommendation: Add a pause modifier which allows Admin to stop interaction with Auctioned Vault function like payBase in case of emergency

Ignore pair on ongoing auction has no impact

Contract: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L150

Issue: If setIgnoredPair function is called to ignore a pair on which a auction is already live then it has no impact on live auction

Recommendation: Display an error to Admin mentioning that pair is already running live auction in a vault. If still required then have a boolean param which can forcefully ignore this pair

Check non existing vault to save gas

Contract: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L180

Issue: auction function will fail at later stage even if vault id provided by user does not exist. This waste gas

Recommendation: Add below check:

 DataTypes.Vault memory vault = cauldron.vaults(vaultId);
 require(vault.owner!=address(0), "Vault not found");

maxBaseIn cannot be 0

Contract: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L286

Issue: If maxBaseIn is provided 0 value while calling payBase function, it means user is paying for 0 debt which does not make sense and should be reverted immediately to prevent gas. Same applies for payFYToken function

Recommendation: Add below check

require(maxBaseIn!=0,"Incorrect value");