Vader Protocol contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

The mintSynth() function is callable by any user and creates a synthetic asset against foreignAsset if it does not already exist. The protocol expects a user to first approve the contract as a spender before calling mintSynth(). However, any arbitrary user could monitor the blockchain for contract approvals that match VaderPoolV2.sol and effectively frontrun their call to mintSynth() by setting the to argument to their own address. As a result, the nativeDeposit amount is transferred from the victim, and a synthetic asset is minted and finally transferred to the malicious user who is represented by the to address.

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L153-L194

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider removing the from argument in mintSynth() and update the safeTransferFrom() call to instead utilise msg.sender as the from argument in transferFrom.

Handle

leastwood

Vulnerability details

Impact

_calculateVaderPrice and _calculateUSDVPrice are used indirectly by USDV.sol to calculate the mint and burn amounts for the respective actions. totalUSD and totalVader/totalUSDV are calculated by iterating through each token pair, where the ratio between the two amounts represents the exchange rate. Each token pair is weighted according to the amount of liquidity is has, relative to other token pairs.

A token pair which is priced at a higher USD/Vader value will impact the underlying ratio more and more as the price moves in either direction. Hence, this further reduces the impact of liquidity weightings and potentially leads to larger swings in price. This also affects how USDV is priced.

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L191-L219

function _calculateVaderPrice(
    uint256[] memory liquidityWeights,
    uint256 totalVaderLiquidityWeight
) internal view returns (uint256) {
    uint256 totalUSD;
    uint256 totalVader;
    uint256 totalPairs = vaderPairs.length;

    for (uint256 i; i < totalPairs; ++i) {
        IUniswapV2Pair pair = vaderPairs[i];
        ExchangePair storage pairData = twapData[address(pair)];

        uint256 foreignPrice = getChainlinkPrice(pairData.foreignAsset);

        totalUSD +=
            (foreignPrice * liquidityWeights[i]) /
            totalVaderLiquidityWeight;

        totalVader +=
            (pairData
                .nativeTokenPriceAverage
                .mul(pairData.foreignUnit)
                .decode144() * liquidityWeights[i]) /
            totalVaderLiquidityWeight;
    }

    // NOTE: Accuracy of VADER & USDV is 18 decimals == 1 ether
    return (totalUSD * 1 ether) / totalVader;
}

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L385-L413

function _calculateUSDVPrice(
    uint256[] memory liquidityWeights,
    uint256 totalUSDVLiquidityWeight
) internal view returns (uint256) {
    uint256 totalUSD;
    uint256 totalUSDV;
    uint256 totalPairs = usdvPairs.length;

    for (uint256 i; i < totalPairs; ++i) {
        IERC20 foreignAsset = usdvPairs[i];
        ExchangePair storage pairData = twapData[address(foreignAsset)];

        uint256 foreignPrice = getChainlinkPrice(address(foreignAsset));

        totalUSD +=
            (foreignPrice * liquidityWeights[i]) /
            totalUSDVLiquidityWeight;

        totalUSDV +=
            (pairData
                .nativeTokenPriceAverage
                .mul(pairData.foreignUnit)
                .decode144() * liquidityWeights[i]) /
            totalUSDVLiquidityWeight;
    }

    // NOTE: Accuracy of VADER & USDV is 18 decimals == 1 ether
    return (totalUSD * 1 ether) / totalUSDV;
}

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider summing the ratio between the USD and Vader/USDV amounts (in each iteration) when calculating the price of Vader and USDV. The average of this summed ratio can be taken to find an exchange rate truly weighted by liquidity and not impacted by the total price of any one token pair being tracked. This should ensure, higher priced assets such as WBTC do not have a larger price impact when it moves against the underlying asset being tracked.

Handle

leastwood

Vulnerability details

Impact

The USDV.mint function queries the price of Vader from the LiquidityBasedTwap contract. The calculation to determine uAmount in mint is actually performed incorrectly. uAmount = (vPrice * vAmount) / 1e18; will return the USD amount for the provided Vader as vPrice is denominated in USD/Vader. This uAmount is subsequently used when minting tokens for the user (locked for a period of time) and fee to the contract owner.

This same issue also applies to how vAmount = (uPrice * uAmount) / 1e18; is calculated in USDV.burn.

This is a severe issue, as the mint and burn functions will always use an incorrect amount of tokens, leading to certain loss by either the protocol (if the user profits) or the user (if the user does not profit).

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/tokens/USDV.sol#L66-L98

function mint(uint256 vAmount)
    external
    onlyWhenNotLocked
    returns (uint256 uAmount)
{
    uint256 vPrice = lbt.getVaderPrice();

    vader.transferFrom(msg.sender, address(this), vAmount);
    vader.burn(vAmount);

    uAmount = (vPrice * vAmount) / 1e18;

    if (cycleTimestamp <= block.timestamp) {
        cycleTimestamp = block.timestamp + 24 hours;
        cycleMints = uAmount;
    } else {
        cycleMints += uAmount;
        require(
            cycleMints <= dailyLimit,
            "USDV::mint: 24 Hour Limit Reached"
        );
    }

    if (exchangeFee != 0) {
        uint256 fee = (uAmount * exchangeFee) / _MAX_BASIS_POINTS;
        uAmount = uAmount - fee;
        _mint(owner(), fee);
    }

    _mint(address(this), uAmount);

    _createLock(LockTypes.USDV, uAmount);
}

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/tokens/USDV.sol#L100-L120

function burn(uint256 uAmount)
    external
    onlyWhenNotLocked
    returns (uint256 vAmount)
{
    uint256 uPrice = lbt.getUSDVPrice();

    _burn(msg.sender, uAmount);

    vAmount = (uPrice * uAmount) / 1e18;

    if (exchangeFee != 0) {
        uint256 fee = (vAmount * exchangeFee) / _MAX_BASIS_POINTS;
        vAmount = vAmount - fee;
        vader.mint(owner(), fee);
    }

    vader.mint(address(this), vAmount);

    _createLock(LockTypes.VADER, vAmount);
}

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider utilising both getVaderPrice and getUSDVPrice when calculating the expected uAmount and vAmount to mint or burn. To calculate uAmount in mint, vPrice should be denominated in USDV/Vader. To calculate vAmount in burn, uPrice should be denominated in Vader/USDV. It would be useful to add unit tests to test this explicitly as it is expected that users will interact with the USDV.sol contract frequently.

Handle

leastwood

Vulnerability details

Impact

The LiquidityBasedTWAP contract attempts to accurately track the price of VADER and USDV while still being resistant to flash loan manipulation and short-term volatility. The previousPrices array is meant to track the last queried price for the two available paths, namely VADER and USDV.

The setupVader function configures the VADER token by setting previousPrices and adding a token pair. However, syncVaderPrice does not update previousPrices after syncing, causing currentLiquidityEvaluation to be dependent on the initial price for VADER. As a result, liquidity weightings do not accurately reflect the current and most up to date price for VADER.

This same issue also affects how USDV calculates currentLiquidityEvaluation.

This issue is of high risk and heavily impacts the accuracy of the TWAP implementation as the set price for VADER/USDV diverges from current market prices. For example, as the Chainlink oracle price and initial price for VADER diverge, currentLiquidityEvaluation will begin to favour either on-chain or off-chain price data depending on which price result is greater. The following calculation for currentLiquidityEvaluation outlines this behaviour.

currentLiquidityEvaluation =
    (reserveNative * previousPrices[uint256(Paths.VADER)]) +
    (reserveForeign * getChainlinkPrice(pairData.foreignAsset));

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L150-L189

function _updateVaderPrice(
    IUniswapV2Pair pair,
    ExchangePair storage pairData,
    uint256 timeElapsed
) internal returns (uint256 currentLiquidityEvaluation) {
    bool isFirst = pair.token0() == vader;

    (uint256 reserve0, uint256 reserve1, ) = pair.getReserves();

    (uint256 reserveNative, uint256 reserveForeign) = isFirst
        ? (reserve0, reserve1)
        : (reserve1, reserve0);

    (
        uint256 price0Cumulative,
        uint256 price1Cumulative,
        uint256 currentMeasurement
    ) = UniswapV2OracleLibrary.currentCumulativePrices(address(pair));

    uint256 nativeTokenPriceCumulative = isFirst
        ? price0Cumulative
        : price1Cumulative;

    unchecked {
        pairData.nativeTokenPriceAverage = FixedPoint.uq112x112(
            uint224(
                (nativeTokenPriceCumulative -
                    pairData.nativeTokenPriceCumulative) / timeElapsed
            )
        );
    }

    pairData.nativeTokenPriceCumulative = nativeTokenPriceCumulative;

    pairData.lastMeasurement = currentMeasurement;

    currentLiquidityEvaluation =
        (reserveNative * previousPrices[uint256(Paths.VADER)]) +
        (reserveForeign * getChainlinkPrice(pairData.foreignAsset));
}

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L221-L235

function setupVader(
    IUniswapV2Pair pair,
    IAggregatorV3 oracle,
    uint256 updatePeriod,
    uint256 vaderPrice
) external onlyOwner {
    require(
        previousPrices[uint256(Paths.VADER)] == 0,
        "LBTWAP::setupVader: Already Initialized"
    );

    previousPrices[uint256(Paths.VADER)] = vaderPrice;

    _addVaderPair(pair, oracle, updatePeriod);
}

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider updating previousPrices[uint256(Paths.VADER)] and previousPrices[uint256(Paths.USDV)] after syncing the respective prices for the two tokens. This will ensure the most up to date price is used when evaluating liquidity for all available token pairs.

Handle

leastwood

Vulnerability details

Impact

The _addVaderPair function is called by the onlyOwner role. The relevant data in the twapData mapping is set by querying the respective liquidity pool and Chainlink oracle. totalLiquidityWeight for the VADER path is also incremented by the pairLiquidityEvaluation amount (calculated within _addVaderPair). If a user then calls syncVaderPrice, the recently updated totalLiquidityWeight will be taken into consideration when iterating through all token pairs eligible for price updates to calculate the liquidity weight for each token pair. This data is stored in pastTotalLiquidityWeight and pastLiquidityWeights respectively.

As a result, newly added token pairs will increase pastTotalLiquidityWeight while leaving pastLiquidityWeights underrepresented. This only occurs if syncVaderPrice is called before the update period for the new token has not been passed.

This issue also affects how the price for USDV is synced.

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L299

function _addVaderPair(
    IUniswapV2Pair pair,
    IAggregatorV3 oracle,
    uint256 updatePeriod
) internal {
    require(
        updatePeriod != 0,
        "LBTWAP::addVaderPair: Incorrect Update Period"
    );

    require(oracle.decimals() == 8, "LBTWAP::addVaderPair: Non-USD Oracle");

    ExchangePair storage pairData = twapData[address(pair)];

    bool isFirst = pair.token0() == vader;

    (address nativeAsset, address foreignAsset) = isFirst
        ? (pair.token0(), pair.token1())
        : (pair.token1(), pair.token0());

    oracles[foreignAsset] = oracle;

    require(nativeAsset == vader, "LBTWAP::addVaderPair: Unsupported Pair");

    pairData.foreignAsset = foreignAsset;
    pairData.foreignUnit = uint96(
        10**uint256(IERC20Metadata(foreignAsset).decimals())
    );

    pairData.updatePeriod = updatePeriod;
    pairData.lastMeasurement = block.timestamp;

    pairData.nativeTokenPriceCumulative = isFirst
        ? pair.price0CumulativeLast()
        : pair.price1CumulativeLast();

    (uint256 reserve0, uint256 reserve1, ) = pair.getReserves();

    (uint256 reserveNative, uint256 reserveForeign) = isFirst
        ? (reserve0, reserve1)
        : (reserve1, reserve0);

    uint256 pairLiquidityEvaluation = (reserveNative *
        previousPrices[uint256(Paths.VADER)]) +
        (reserveForeign * getChainlinkPrice(foreignAsset));

    pairData.pastLiquidityEvaluation = pairLiquidityEvaluation;

    totalLiquidityWeight[uint256(Paths.VADER)] += pairLiquidityEvaluation;

    vaderPairs.push(pair);

    if (maxUpdateWindow < updatePeriod) maxUpdateWindow = updatePeriod;
}

https://github.com/code-423n4/2021-12-vader/blob/main/contracts/lbt/LiquidityBasedTWAP.sol#L113-L148

function syncVaderPrice()
    public
    override
    returns (
        uint256[] memory pastLiquidityWeights,
        uint256 pastTotalLiquidityWeight
    )
{
    uint256 _totalLiquidityWeight;
    uint256 totalPairs = vaderPairs.length;
    pastLiquidityWeights = new uint256[](totalPairs);
    pastTotalLiquidityWeight = totalLiquidityWeight[uint256(Paths.VADER)];

    for (uint256 i; i < totalPairs; ++i) {
        IUniswapV2Pair pair = vaderPairs[i];
        ExchangePair storage pairData = twapData[address(pair)];
        uint256 timeElapsed = block.timestamp - pairData.lastMeasurement;

        if (timeElapsed < pairData.updatePeriod) continue;

        uint256 pastLiquidityEvaluation = pairData.pastLiquidityEvaluation;
        uint256 currentLiquidityEvaluation = _updateVaderPrice(
            pair,
            pairData,
            timeElapsed
        );

        pastLiquidityWeights[i] = pastLiquidityEvaluation;

        pairData.pastLiquidityEvaluation = currentLiquidityEvaluation;

        _totalLiquidityWeight += currentLiquidityEvaluation;
    }

    totalLiquidityWeight[uint256(Paths.VADER)] = _totalLiquidityWeight;
}

As shown above, pastTotalLiquidityWeight = totalLiquidityWeight[uint256(Paths.VADER)] loads in the total liquidity weight which is updated when _addVaderPair is called. However, pastLiquidityWeights is calculated by iterating through each token pair that is eligible to be updated.

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider removing the line totalLiquidityWeight[uint256(Paths.VADER)] += pairLiquidityEvaluation; in _addVaderPair so that newly added tokens do not impact upcoming queries for VADER/USDV price data. This should ensure syncVaderPrice and syncUSDVPrice cannot be manipulated when adding new tokens.