Yield Witch v2 contest - antonttc's results

Lines of code

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

Vulnerability details

Impact

might lead to systematic debt. Cause errors for liquidators to run normally.

Proof of Concept

In the function auction, there is on input validation around whether the to is address(0) or not. and if the auctioneerReward is set to an value > 0 (as default), each liquidate call will call Join module to pay out to auctioneer with the following line:

if (auctioneerCut > 0) {
    ilkJoin.exit(auction_.auctioneer, auctioneerCut.u128());
}

This line will revert if auctioneer is set to address(0) on some tokens (revert on transferring to address(0) is a default behaviour of the OpenZeppelin template). So if someone start an auction with to = address(0), this auction becomes un-liquidatable.

A malicious user can run a bot to monitor his own vault, and if the got underwater and they don’t have enough collateral to top up, they can immediately start an auction on their own vault and set actioneer to 0 to avoid actually being liquidated, which breaks the design of the system.

Recommended Mitigation Steps

Add check while starting an auction:

function auction(bytes12 vaultId, address to)
    external
    returns (DataTypes.Auction memory auction_)
{
    require (to != address(0), "invalid auctioneer");
		...
}		

Gas Optimisation

1. cache proportion in _calcAuction can save gas from reading storage.

in function _calcAuction, only proportion of the line struct is used, we can use the following syntax to only read that 1 field from the struct and store it locally.

Original Code

// We store the proportion of the vault to auction, which is the whole vault if the debt would be below dust.
DataTypes.Line storage line = lines[vault.ilkId][series.baseId];
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();

After caching

uint64 proportion = (lines[vault.ilkId][series.baseId]).proportion;
uint128 art = uint256(balances.art).wmul(proportion).u128();
if (art < debt.min * (10**debt.dec)) art = balances.art;
uint128 ink = (art == balances.art)
    ? balances.ink
    : uint256(balances.ink).wmul(proportion).u128();

Gas saving: around 100 gas.

auction: 91148 ⇒ 91031

calcPayout: 19151 ⇒ 19034

2.Simplify proportionNow calculation in _calcPayout

in the branch (elapsed < duration) while calculating proportionNo, two small optimisation can be applied:

1. unchecked block can be use since 1e18 - initialProportion cannot underflow
2. don’t need to use WMul and wDiv on elapsed and duration, they are the same unit and the /WAD and *WAD cancel out

The function can be simplified as follow

if (duration == type(uint32).max) { // same
    proportionNow = initialProportion;
} else if (elapsed > duration) {
    proportionNow = 1e18; // same
} else {
    unchecked {
        proportionNow =
        uint256(initialProportion) +
        (uint256(1e18 - initialProportion) * elapsed / duration);
    }
}

Gas saving: 400 gas saved on _calcPayout .

• calcPayout: 19151 ⇒ 18713
• payBase: 26694 ⇒ 26256
• payFYToken:27910 ⇒ 27472

3. Simplify inkAtEnd calculation in _calcPayout

Similar to the previous report: wMul and wDiv cancelled out and make it more expensive to do the additional *WAD and /WAD. we can save this without lost of precision.

Original code:

uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);

Simplified code:

uint256 inkAtEnd = uint256(artIn) * auction_.ink / auction_.art;

Note: need to apply multiply before division to prevent lost of precision.

Gas savings: around 220 gas on _calcPayout

• calcPayout: 19151 ⇒ 18945
• payBase: 26694 ⇒ 26488
• payFYToken:27910 ⇒ 27704

4. Avoid external call in payBase

In the current code, it calculates the real baseIn by calling cauldron.debtToBase and pass in artIn . the variable artIn is actually derived by parameter maxBaseIn, so converting it back and forth (base to debt to base) is unnecessary, if artIn is not changed by the following line:

// If offering too much base, take only the necessary.
artIn = artIn > auction_.art ? auction_.art : artIn;

A overal more efficient implementation is

if (artIn > auction_.art) {
      artIn = auction_.art;
      baseIn = cauldron.debtToBase(auction_.seriesId, artIn);
  } else {
      baseIn = maxBaseIn;
  }

So we can avoid calling debtToBase again if artIn is not higher than collateral in the vault, which should be most cases.

Gas savings: around 450 gas on payBase

• payBase : 26694 ⇒ 26134

5. Simplify setLimit

The current code of setLimit involves unnecessary operation on reading the sum value and setting the same value back to storage. This can be changed to only update the max value.

Original

limits[ilkId][baseId] = DataTypes.Limits({
    max: max,
    sum: limits[ilkId][baseId].sum // sum is initialized at zero, and doesn't change when changing any ilk parameters
});

Simplified version. also increase readability.

limits[ilkId][baseId].max = max;

Gas saving:

• setLimit: 27218 ⇒ 27100 (118 gas)

6. Enable Optimizer

the current config doesn’t enable optimizer, enabling it can save around 200~1200 gas, depends on functions.

update to foundry.toml

optimizer = true
optimizer_runs = 1000000

Gas savings: (most significant)

• auction: 91148 ⇒ 89863 (around 1200 gas)
• calcPayout : 19151 ⇒ 18269 (900 gas)
• payBase: 26694 ⇒ 25296 (1400 gas)
• payFYToken: 27910 ⇒ 26797 (1100 gas)