Vader Protocol contest - cmichel's results

Handle

cmichel

Vulnerability details

The VaderPoolV2.mintSynth implicitly performs a "native -> foreign" swap using VaderMath.calculateSwap(nativeDeposit,reserveNative,reserveForeign), the resulting amount will be minted as synths instead of transferred out as foreign tokens.

The calculateSwap calculation, therefore, depends on the current reserves and the mintSynth does not allow specifying any minSynthAmount for the caller to make sure that they cannot be sandwich attacked. It's basically doing a trade with a slippage check always set to 100%.

Impact

Any mintSynth call can be sandwich attacked to make a profit.

A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.

• Attacker frontruns mintSynth by performing a standard swap, trading native assets to foreign assets, leading to a worse synth price for the victim
• Victim performs the mint synth, depositing native tokens (receiving synths at a worse price)
• Attacker now sells the received foreign assets from the first step into the increased native reserves for a profit

Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the user's loss.

Recommended Mitigation Steps

Add minimum return amount checks. Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.

Handle

cmichel

Vulnerability details

The 3-path hop in VaderRouter._swap is supposed to first swap foreign assets to native assets, and then the received native assets to different foreign assets again.

The pool.swap(nativeAmountIn, foreignAmountIn) accepts the foreign amount as the second argument. The code however mixes these positional arguments up and tries to perform a pool0 foreign -> native swap by using the foreign amount as the native amount:

function _swap(
    uint256 amountIn,
    address[] calldata path,
    address to
) private returns (uint256 amountOut) {
    if (path.length == 3) {
      // ...
      // @audit calls this with nativeAmountIn = amountIn. but should be foreignAmountIn (second arg)
      return pool1.swap(0, pool0.swap(amountIn, 0, address(pool1)), to);
    }
}

// @audit should be this instead
return pool1.swap(pool0.swap(0, amountIn, address(pool1)), 0, to);

Impact

All 3-path swaps through the VaderRouter fail in the pool check when require(nativeAmountIn = amountIn <= nativeBalance - nativeReserve = 0) is checked, as foreign amount is sent but native amount is specified.

Recommended Mitigation Steps

Use return pool1.swap(pool0.swap(0, amountIn, address(pool1)), 0, to); instead.

Handle

cmichel

Vulnerability details

The 3-path hop in VaderRouter.calculateOutGivenIn is supposed to first swap foreign assets to native assets in pool0, and then the received native assets to different foreign assets again in pool1.

The first argument of VaderMath.calculateSwap(amountIn, reserveIn, reserveOut) must refer to the same token as the second argument reserveIn. The code however mixes these positions up and first performs a swap in pool1 instead of pool0:

function calculateOutGivenIn(uint256 amountIn, address[] calldata path)
    external
    view
    returns (uint256 amountOut)
{
  if(...) {
  } else {
    return
        VaderMath.calculateSwap(
            VaderMath.calculateSwap(
                // @audit the inner trade should not be in pool1 for a forward swap. amountIn foreign => next param should be foreignReserve0
                amountIn,
                nativeReserve1,
                foreignReserve1
            ),
            foreignReserve0,
            nativeReserve0
        );
  }

 /** @audit instead should first be trading in pool0!
    VaderMath.calculateSwap(
        VaderMath.calculateSwap(
            amountIn,
            foreignReserve0,
            nativeReserve0
        ),
        nativeReserve1,
        foreignReserve1
    );
  */

Impact

All 3-path swaps computations through VaderRouter.calculateOutGivenIn will return the wrong result. Smart contracts or off-chain scripts/frontends that rely on this value to trade will have their transaction reverted, or in the worst case lose funds.

Recommended Mitigation Steps

Return the following code instead which first trades in pool0 and then in pool1:

return
  VaderMath.calculateSwap(
      VaderMath.calculateSwap(
          amountIn,
          foreignReserve0,
          nativeReserve0
      ),
      nativeReserve1,
      foreignReserve1
  );

Handle

cmichel

Vulnerability details

The TWAPOracle.registerPair function takes in a factory and (token0, token1). The function accepts a _factory argument which means any Uniswap-like factory can be used.

When using the actual Uniswap factory's IUniswapV2Factory(factory).getPair(token0, token1) call, it could be that the token0 and token1 are reversed as it ignores the order.

Meaning, the price0/1CumulativeLast could also be reversed as it matches the internal order. The code however pushes the _pairs assuming that the internal price0CumulativeLast, price1CumulativeLast order matches the order of the function arguments token0, token1.

_pairs.push(
    PairData({
        pair: pairAddr,
        token0: token0,
        token1: token1,
        price0CumulativeLast: price0CumulativeLast,
        price1CumulativeLast: price1CumulativeLast,
        blockTimestampLast: blockTimestampLast,
        price0Average: FixedPoint.uq112x112({_x: 0}),
        price1Average: FixedPoint.uq112x112({_x: 0})
    })
);

Impact

The prices could be inverted which leads to the oracle providing wrong prices.

Recommended Mitigation Steps

It should be checked if Uniswap's internal order matches the order of the token0/1 function arguments. If not, the cumulative prices must be swapped.

// pseudocode
IUniswapV2Pair pair = IUniswapV2Pair(
    IUniswapV2Factory(factory).getPair(token0, token1)
);
pairAddr = address(pair);
price0CumulativeLast = pair.price0CumulativeLast();
price1CumulativeLast = pair.price1CumulativeLast();
(price0CumulativeLast, price1CumulativeLast) = token0 == pair.token0() ? (price0CumulativeLast, price1CumulativeLast) : (price1CumulativeLast, price0CumulativeLast);

The same issue exists in update

Handle

cmichel

Vulnerability details

The VaderPoolV2 mintFungible and mintSynth functions perform an unsafe nativeAsset.safeTransferFrom(from, address(this), nativeDeposit) with a parameter-specified from address.

Note that these functions are not called by the Router, they are directly called on the pool. Therefore, users will usually be required to send two transactions, a first one approving the pool, and then a second one for the actual mintSynth.

An attacker can frontrun the mintSynth(IERC20 foreignAsset, uint256 nativeDeposit, address from, address to) function, use the same from=victim parameter but change the to parameter to the attacker.

Impact

It's possible to frontrun victims stealing their native token deposits and receiving synths / fungible tokens.

Recommended Mitigation Steps

Remove the from parameter and always perform the safeTransferFrom call with from=msg.sender.

Handle

cmichel

Vulnerability details

The GovernorAlpha's council cannot veto proposals that perform a call to the contract itself. This can be exploited by malicious proposal creators by appending a new call at the end of their proposal that simply calls an innocent function like GovernorAlpha.votingDelay().

Impact

The veto procedure can easily be circumvented, making the council unable to veto.

Recommended Mitigation Steps

The veto check must be further restricted by specifying the actual function selector that is not allowed to be vetoed, like changeCouncil.

Handle

cmichel

Vulnerability details

The LinearVesting.vestFor function (which is called by Converter) reverts if there already exists a vest for the user:

 require(
    vest[user].amount == 0,
    "LinearVesting::selfVest: Already a vester"
);

There's an attack where a griefer frontruns the vestFor call and instead vests the smallest unit of VADER for the user. The original transaction will then revert and the vest will be denied

Recommended Mitigation Steps

There are several ways to mitigate this. The most involved one would be to allow several separate vestings per user.

Handle

cmichel

Vulnerability details

The TWAPOracle.getRate function simply performs an integer division to compute the rate.

function getRate() public view returns (uint256 result) {
    uint256 tUSDInUSDV = consult(USDV);
    uint256 tUSDInVader = consult(VADER);
    // @audit shouldn't this scale by 1e18 first? otherwise easily 0
    result = tUSDInUSDV / tUSDInVader;
}

It should first be scaled by some value, for example, 1e18.

Impact

The rate has no decimal precision and if tUSDInVader > tUSDInUSDV, the rate will always be zero.

The usdvtoVader and vaderToUsdv functions will return incorrect values.

Recommended Mitigation Steps

// return as a rate with 18 decimals instead
result = tUSDInUSDV * 1e18 / tUSDInVader;

Handle

cmichel

Vulnerability details

The TWAPOracle.consult function is unclear to the auditor. It seems to iterate through all registered pairs that share the token parameter (USDV or VADER) and then sums up the foreign token pair per token price. And divides this sum (sumNative) by the summed-up USD price of these foreign token pairs (sumUSD).

I think the idea is to create some kind of average price but doing it like this does not seem to be effective because large prices are weighted a lot stronger than low prices.

Example

Assume there are 3 USDV pairs registered: (ETH, DAI, USDC).

Oracle Price: USDV/ETH 4500, USDV/DAI 1, USDV/USDC 1 Pool price: USDV/ETH 4500, USDV/DAI 10, USDV/USDC 10

Even though the DAI and USDC pool prices are off by 10x, the final result is 4502/4520 = 0.996017699 very close to a price of 1.0 which seems strange.

Recommended Mitigation Steps

Document how the algorithm works and make sure it's correct. Resolve the TODO.