Vader Protocol contest - WatchPug's results

Handle

WatchPug

Vulnerability details

The current implementation of Vader protocol provides impermanent loss coverage calculated as below:

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex/math/VaderMath.sol#L73-L93

function calculateLoss(
    uint256 originalVader,
    uint256 originalAsset,
    uint256 releasedVader,
    uint256 releasedAsset
) public pure returns (uint256 loss) {
    //
    // TODO: Vader Formula Differs https://github.com/vetherasset/vaderprotocol-contracts/blob/main/contracts/Utils.sol#L347-L356
    //

    // [(A0 * P1) + V0]
    uint256 originalValue = ((originalAsset * releasedVader) /
        releasedAsset) + originalVader;

    // [(A1 * P1) + V1]
    uint256 releasedValue = ((releasedAsset * releasedVader) /
        releasedAsset) + releasedVader;

    // [(A0 * P1) + V0] - [(A1 * P1) + V1]
    if (originalValue > releasedValue) loss = originalValue - releasedValue;
}

An attacker may exploit this by adding liquidity and manipulating the price of the pool (with flash loans) to get IL coverage from the protocol.

PoC

Given:

• The Vader pool for BTC-USDV is newly created, with nearly 0 liquidity.
• The price of BTC in USDV is stable, no impermanent loss.

The attacker can:

1. Add liquidity with 1M USDV and 10 BTC;
2. 30 days later, do the following in one transaction:
   1. borrow a flash loan of 200 BTC, swap 20 BTC to USDV, repeat for 10 times;
   2. current reserves: 189055 USDV and 210 BTC, current loss: 630,891 USDV;
   3. remove all liquidity, repay flash loan, profit for the coveredLoss = 630,891 * 30 / 365 = 52k USDV.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex/pool/BasePool.sol#L161-L163

uint256 totalLiquidityUnits = totalSupply;
if (totalLiquidityUnits == 0)
    liquidity = nativeDeposit; // TODO: Contact ThorChain on proper approach

In the current implementation, the first liquidity takes the nativeDeposit amount and uses it directly.

However, since this number (totalLiquidityUnits) will later be used for computing the liquidity issued for future addLiquidity using calculateLiquidityUnits.

A malicious user can add liquidity with only 1 wei USDV and making it nearly impossible for future users to add liquidity to the pool.

Recomandation

Uni v2 solved this problem by sending the first 1000 tokens to the zero address.

The same should work here, i.e., on first mint (totalLiquidityUnits == 0), lock some of the first minter's tokens by minting ~1% of the initial amount to the zero address instead of to the first minter.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/VaderPoolV2.sol#L284-L335

function mintFungible(
        IERC20 foreignAsset,
        uint256 nativeDeposit,
        uint256 foreignDeposit,
        address from,
        address to
    ) external override nonReentrant returns (uint256 liquidity) {
        IERC20Extended lp = wrapper.tokens(foreignAsset);

        require(
            lp != IERC20Extended(_ZERO_ADDRESS),
            "VaderPoolV2::mintFungible: Unsupported Token"
        );

        (uint112 reserveNative, uint112 reserveForeign, ) = getReserves(
            foreignAsset
        ); // gas savings

        nativeAsset.safeTransferFrom(from, address(this), nativeDeposit);
        foreignAsset.safeTransferFrom(from, address(this), foreignDeposit);

        PairInfo storage pair = pairInfo[foreignAsset];
        uint256 totalLiquidityUnits = pair.totalSupply;
        if (totalLiquidityUnits == 0) liquidity = nativeDeposit;
        else
            liquidity = VaderMath.calculateLiquidityUnits(
                nativeDeposit,
                reserveNative,
                foreignDeposit,
                reserveForeign,
                totalLiquidityUnits
            );

        require(
            liquidity > 0,
            "VaderPoolV2::mintFungible: Insufficient Liquidity Provided"
        );

        pair.totalSupply = totalLiquidityUnits + liquidity;

        _update(
            foreignAsset,
            reserveNative + nativeDeposit,
            reserveForeign + foreignDeposit,
            reserveNative,
            reserveForeign
        );

        lp.mint(to, liquidity);

        emit Mint(from, to, nativeDeposit, foreignDeposit);
    }

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/VaderPoolV2.sol#L126-L167

Funds are transferred from the from parameter, and the output tokens are transferred to the to parameter, both passed by the caller without proper access control.

Impact

This issue allows anyone to call mintFungible() and mintSynth() and steal almost all their wallet balances for all the users who have approved the contract before.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/VaderPoolV2.sol#L126-L155

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/VaderPoolV2.sol#L179-L197

Given that mintSynth() and burnSynth() will issue and redeem assets based on the price of the pool (reserves), and they will create price impact based on the volume being minted and burnt.

However, the current implementation provides no parameter for slippage control, making them vulnerable to front-run attacks. Especially for transactions with rather large volumes.

Recommendation

Consider adding a minAmountOut parameter.

Handle

WatchPug

Vulnerability details

Per the document:

It also is capable of using liquidity units as collateral for synthetic assets, of which it will always have guaranteed redemption liquidity for.

However, in the current implementation, Synth tokens are minted based on the calculation result. While nativeDeposit be added to the reserve, reserveForeign will remain unchanged, not deducted nor locked.

Making it possible for Synth tokens to get over-minted.

PoC

• The Vader pool for BTC-USDV is newly created, with nearly 0 liquidity.

1. Alice add liquidity with 100,000 USDV and 1 BTC;
2. Bob mintSynth() with 100,000 USDV, got 0.25 BTC vSynth;
3. Alice remove all the liquidity received at step 1, got all the 200k USDV and 1 BTC.

The 0.25 BTC vSynth held by Bob is now backed by nothing and unable to be redeemed.

This also makes it possible for a sophisticated attacker to steal funds from the Vader pool.

The attacker may do the following in one transaction:

1. Add liquidity with 10 USDV and 10,000 BTC (flash loan);
2. Call mintSynth() with 10 USDV, repeat for 10 times, got 1461 BTC vSynth;
3. Remove liquidity and repay flash loan, keep the 1461 BTC vSynth;
4. Wait for other users to add liquidity and when the BTC reserve is sufficient, call burnSynth() to steal USDV from the pool.

Handle

WatchPug

Vulnerability details

The current design/implementation of Vader pool allows users to addLiquidity using arbitrary amounts instead of a fixed ratio of amounts in comparison to Uni v2.

We believe this design is flawed and it essentially allows anyone to manipulate the price of the pool easily and create an arbitrage opportunity at the cost of all other liquidity providers.

An attacker can exploit this by adding liquidity in extreme amounts and drain the funds from the pool.

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/VaderPoolV2.sol#L284-L335

function mintFungible(
    IERC20 foreignAsset,
    uint256 nativeDeposit,
    uint256 foreignDeposit,
    address from,
    address to
) external override nonReentrant returns (uint256 liquidity) {
    IERC20Extended lp = wrapper.tokens(foreignAsset);

    require(
        lp != IERC20Extended(_ZERO_ADDRESS),
        "VaderPoolV2::mintFungible: Unsupported Token"
    );

    (uint112 reserveNative, uint112 reserveForeign, ) = getReserves(
        foreignAsset
    ); // gas savings

    nativeAsset.safeTransferFrom(from, address(this), nativeDeposit);
    foreignAsset.safeTransferFrom(from, address(this), foreignDeposit);

    PairInfo storage pair = pairInfo[foreignAsset];
    uint256 totalLiquidityUnits = pair.totalSupply;
    if (totalLiquidityUnits == 0) liquidity = nativeDeposit;
    else
        liquidity = VaderMath.calculateLiquidityUnits(
            nativeDeposit,
            reserveNative,
            foreignDeposit,
            reserveForeign,
            totalLiquidityUnits
        );

    require(
        liquidity > 0,
        "VaderPoolV2::mintFungible: Insufficient Liquidity Provided"
    );

    pair.totalSupply = totalLiquidityUnits + liquidity;

    _update(
        foreignAsset,
        reserveNative + nativeDeposit,
        reserveForeign + foreignDeposit,
        reserveNative,
        reserveForeign
    );

    lp.mint(to, liquidity);

    emit Mint(from, to, nativeDeposit, foreignDeposit);
}

PoC

Given:

• A Vader pool with 100,000 USDV and 1 BTC;
• The totalPoolUnits is 100.

The attacker can do the following in one transaction:

1. Add liquidity with 100,000 USDV and 0 BTC, get 50 liquidityUnits, representing 1/3 shares of the pool;
2. Swap 0.1 BTC to USDV, repeat for 5 times; spent0.5 BTC and got 62163.36 USDV;
3. Remove liquidity, get back 45945.54 USDV and 0.5 BTC; profit for: 62163.36 + 45945.54 - 100000 = 8108.9 USDV.

Handle

WatchPug

Vulnerability details

The current formula to calculate the amountOut for a swap is:

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex/math/VaderMath.sol#L99-L111

function calculateSwap(
    uint256 amountIn,
    uint256 reserveIn,
    uint256 reserveOut
) public pure returns (uint256 amountOut) {
    // x * Y * X
    uint256 numerator = amountIn * reserveIn * reserveOut;

    // (x + X) ^ 2
    uint256 denominator = pow(amountIn + reserveIn);

    amountOut = numerator / denominator;
}

We believe the design (the formula) is wrong and it will result in unexpected and unfavorable outputs.

Specifically, if the amountIn is larger than the reserveIn, the amountOut starts to decrease.

PoC

Given:

• A USDV-BTC Vader pool with the reserves of 200,000 USDV and 2 BTC.

1. If Alice swap 2 BTC for USDV, will get 50000 USDV as output;
2. If Bob swap 2.1 BTC for USDV, will only get 49970.25 USDV as output;
3. If Carol swap 2.2 BTC for USDV, will only get 49886.62 USDV as output.

For the same pool reserves, paying more for less output token is unexpected and unfavorable.

Handle

WatchPug

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom(), E.g Vader token.

In the current implementation, BasePoolV2.sol#mint() assumes that the received amount is the same as the transfer amount, and uses it to calculate liquidity units.

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/pool/BasePoolV2.sol#L168-L229

function mint(
    IERC20 foreignAsset,
    uint256 nativeDeposit,
    uint256 foreignDeposit,
    address from,
    address to
)
    external
    override
    nonReentrant
    onlyRouter
    supportedToken(foreignAsset)
    returns (uint256 liquidity)
{
    (uint112 reserveNative, uint112 reserveForeign, ) = getReserves(
        foreignAsset
    ); // gas savings

    nativeAsset.safeTransferFrom(from, address(this), nativeDeposit);
    foreignAsset.safeTransferFrom(from, address(this), foreignDeposit);

    PairInfo storage pair = pairInfo[foreignAsset];
    uint256 totalLiquidityUnits = pair.totalSupply;
    if (totalLiquidityUnits == 0) liquidity = nativeDeposit;
    else
        liquidity = VaderMath.calculateLiquidityUnits(
            nativeDeposit,
            reserveNative,
            foreignDeposit,
            reserveForeign,
            totalLiquidityUnits
        );

    require(
        liquidity > 0,
        "BasePoolV2::mint: Insufficient Liquidity Provided"
    );

    uint256 id = positionId++;

    pair.totalSupply = totalLiquidityUnits + liquidity;
    _mint(to, id);

    positions[id] = Position(
        foreignAsset,
        block.timestamp,
        liquidity,
        nativeDeposit,
        foreignDeposit
    );

    _update(
        foreignAsset,
        reserveNative + nativeDeposit,
        reserveForeign + foreignDeposit,
        reserveNative,
        reserveForeign
    );

    emit Mint(from, to, nativeDeposit, foreignDeposit);
    emit PositionOpened(from, to, id, liquidity);
}

Recommended

Consider calling balanceOf() to get the actual balances.