Vader Protocol contest - TomFrenchBlockchain's results

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Total loss of user-held USDV which has been approved on VaderPoolV2

Proof of Concept

VaderPoolV2 allows minting of sythentic tokens ("synths") using USDV with the mintSynth function

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

Crucially this function allows a user supplied value for from which specifies where the USDV should be pulled from. An attacker can then provide any address which has a token approval onto VaderPoolV2 for USDV and mint themselves a supported synth - stealing the underlying USDV.

The attacker than then simply burn the synth and sell the USDV to get a safe asset.

Recommended Mitigation Steps

Remove from argument and use msg.sender instead or a more sophisticated access control method.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Possible theft of all user assets with an ERC20 approval on VaderPoolV2

Proof of Concept

The owner of VaderPoolV2 can call the setTokenSupport function which allows the caller to supply any address from which to take the assets to provide the initial liquidity, the owner can also specify who shall receive the resulting LP NFT and so can take ownership over these assets. This call will succeed for any address which has an ERC20 approval on VaderPoolV2 for USDV and foreignAsset.

https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/dex-v2/pool/VaderPoolV2.sol#L442-L474

This in effect gives custody over all assets in user wallets which are approved on VaderPoolV2 to Vader Protocol governance. This is especially problematic in the case of Vader Protocol as there's a single entity (i.e. the Council) which can force through a proposal to steal these assets for themselves with only the timelock giving protection to users, for this reason I give this high severity.

Recommended Mitigation Steps

Enforce that the initial liquidity is provided by the VaderPoolV2 owner.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Invalid values returned from oracle for USDV and VADER prices in situations where the oracle uses more than one foreign asset.

Proof of Concept

The USDV price is calculated as so (for simplicity we'll consider a two pairs):

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/lbt/LiquidityBasedTWAP.sol#L393-L409

totalUSD =  (PriceForeign0InUSD * liquidityWeights[0] + PriceForeign1InUSD * liquidityWeights[1]) / totalUSDVLiquidityWeight;

totalUSD is then the average price of the foreign assets paired against USDV in terms of USD, weighted by the TVL of the relevant liquidity pool

totalUSDV =
  (pairData0
      .nativeTokenPriceAverage
      .mul(pairData0.foreignUnit)
      .decode144() * liquidityWeights[0] +
   pairData1
      .nativeTokenPriceAverage
      .mul(pairData1.foreignUnit)
      .decode144() * liquidityWeights[1]) /
  totalUSDVLiquidityWeight;

// in pseudocode for readability
totalUSDV = (USDVPriceInForeign0 * liquidityWeights[0] + USDVPriceInForeign1 * liquidityWeights[1]) /  totalUSDVLiquidityWeight

totalUSDV is then the average price of USDV in terms of each of the foreign assets, weighted by the TVL of the relevant liquidity pool.

It should be fairly clear that this is the incorrect calculation as all the terms in totalUSDV are in different units - you can't average the price of USDV in ETH with the price of USDV in BTC and get a meaningful result.

It appears that the VADER team intended to calculate the price of USDV in terms of USD through a number of different paired assets and then average them at the end based on the liquidity in each pair but have started averaging too early.

High severity issue as the oracle is crucial for determining the exchange rate between VADER and USDV to be used for IL protection and minting/burning of USDV - an incorrect value will result in the protocol losing significant funds.

Recommended Mitigation Steps

Review the algorithm used for calculating the prices of assets and ensure that it's calculating what you expect.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Invalid values returned from oracle in vast majority of situations

Proof of Concept

The LBT oracle does not properly scale values when calculating prices for VADER or USDV. To show this we consider the simplest case where we expect USDV to return a value of $1 and show that the oracle does not return this value.

Consider the case of the LBT oracle tracking a single USDV-DAI pair where USDV trades 1:1 for DAI and Chainlink reports that DAI is exactly $1. We then work through the lines linked below:

https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/lbt/LiquidityBasedTWAP.sol#L393-L409

For L397 we get a value of 1e8 as Chainlink reports the price of DAI with 8 decimals of accuracy.

foreignPrice = getChainlinkPrice(address(foreignAsset));
foreignPrice = 1e8

We can set liquidityWeights[i] and totalUSDVLiquidityWeight both to 1 as we only consider a single pair so L399-401 becomes

totalUSD = foreignPrice;
totalUSD = 1e8;

L403-408 is slightly more complex but from looking at the links below we can calculate totalUSDV as shown https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/dex-v2/pool/VaderPoolV2.sol#L81-L90 https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/external/libraries/FixedPoint.sol#L137-L160

totalUSDV = pairData
    .nativeTokenPriceAverage
    .mul(pairData.foreignUnit)
    .decode144()
// pairData.nativeTokenPriceAverage == 2**112
// pairData.foreignUnit = 10**18
// decode144(x) = x >> 112
totalUSDV = (2**112).mul(10**18).decode144()
totalUSDV = 10**18

Using totalUSD and totalUSDV we can then calculate the return value of _calculateUSDVPrice

returnValue = (totalUSD * 1 ether) / totalUSDV;

returnValue = 1e8 * 1e18 / 1e18

returnValue = 1e8

For the oracle implementation to be correct we then expect that the Vader protocol to treat values of 1e8 from the oracle to mean USDV is worth $1. However from the lines of code linked below we can safely assume that it is intended to be that values of 1e18 represent $1 rather than 1e8.

https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/tokens/USDV.sol#L76 https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/tokens/USDV.sol#L109

High severity issue as the oracle is crucial for determining the exchange rate between VADER and USDV to be used for IL protection and minting/burning of USDV - an incorrect value will result in the protocol losing significant funds.

Recommended Mitigation Steps

Go over oracle calculation again to ensure that various scale factors are properly accounted for. Some handling of the difference in the number of decimals between the chainlink oracle and the foreign asset should be added.

Build a test suite to ensure that the oracle returns the expected values for simple situations.

Handle

TomFrenchBlockchain

Vulnerability details

(Resubmission as the form crashed apologies if this is a duplicate)

Impact

Impermanent loss protection can be exploited to drain the reserve.

Proof of Concept

In VaderPoolV2.burn we calculate the current losses that the LP has made to impermanent loss.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex-v2/pool/VaderPoolV2.sol#L265-L296

These losses are then refunded to the LP in VADER tokens from the reserve.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex-v2/router/VaderRouterV2.sol#L220

This loss is calculated by the current reserves of the pool so if an LP can manipulate the pool's reserves they can artificially engineer a huge amount of IL in order to qualify for a payout up to the size of their LP position.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex/math/VaderMath.sol#L72-L92

The attack is then as follows.

1. Be an LP for a reasonable period of time (IL protection scales linearly up to 100% after a year)
2. Flashloan a huge amount of one of the pool's assets.
3. Trade against the pool with the flashloaned funds to unbalance it such that your LP position has huge IL.
4. Remove your liquidity and receive compensation from the reserve for the IL you have engineered.
5. Re-add your liquidity back to the pool.
6. Trade against the pool to bring it back into balance.

The attacker now holds the majority of their flashloaned funds (minus slippage/swap fees) along with a large fraction of the value of their LP position in VADER paid out from the reserve. The value of their LP position is unchanged. Given a large enough LP position, the IL protection funds extracted from the reserve will exceed the funds lost to swap fees and the attacker will be able to repay their flashloan with a profit.

This is a high risk issue as after a year any large LP is incentivised and able to perform this attack and drain reserve funds.

Recommended Mitigation Steps

Use a manipulation resistant oracle for the relative prices of the pool's assets (TWAP, etc.)

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Draining of funds from VaderPoolV2

Proof of Concept

See the VaderPool.mintSynth function: https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex-v2/pool/VaderPoolV2.sol#L153-L194

As the pool's reserves can be manipulated through flashloans similar to on UniswapV2 (the slip mechanism can be mitigated by splitting the manipulation over a number of trades), an attacker may set the exchange rate between nativeAsset and synths (calculated from the reserves). An attacker can exploit this to drain funds from the pool.

1. The attacker first flashloans and sells a huge amount of foreignAsset to the pool. The pool now thinks nativeAsset is extremely valuable.
2. The attacker now uses a relatively small amount of nativeAsset to mint synths using VaderPool.mintSynth. As the pool thinks nativeAsset is very valuable the attacker will receive a huge amount of synths.
3. The attacker can now manipulate the pool in the opposite direction by buying up the foreignAsset they sold to the pool. nativeAsset is now back at its normal price, or perhaps artificially low if the attacker wishes.
4. The attacker now burns all of their synths. As nativeAsset is considered much less valuable than at the point the synths were minted it takes a lot more of nativeAsset in order to pay out for the burned synths.

For the price of a flashloan and some swap fees, the attacker has now managed to extract a large amount of nativeAsset from the pool. This process can be repeated as long as it is profitable.

Recommended Mitigation Steps

Tie the exchange rate use for minting/burning synths to a manipulation resistant oracle.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Reserve pays out vastly higher (or lower) IL protection than it should

Proof of Concept

Consider the lines 98 and 102 as shown on the link below:

https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/reserve/VaderReserve.sol#L95-L103

Here we multiply the IL experienced by the LP by a price for USDV or VADER as returned by the LBT. However the price from the oracle is a fixed point number (scaled up by 1e8 or 1e18 depending on the resolution of finding "Oracle returns an improperly scaled USDV/VADER price") and so a fixed scaling factor should be applied to convert back from a fixed point number to a standard integer.

As it stands depending on the branch which is executed, the amount to be reimbursed will be 1e18 times too large or too small. Should the "else" branch be executed the reserve will pay out much in terms of IL protection resulting in severe loss of funds. High severity.

Recommended Mitigation Steps

Apply similar logic to as displayed here:

https://github.com/code-423n4/2021-12-vader/blob/00ed84015d4116da2f9db0c68db6742c89e73f65/contracts/tokens/USDV.sol#L109

Handle

TomFrenchBlockchain

Vulnerability details

Impact

A money pump exists whenever VADER is worth more than $1.

Proof of Concept

When minting USDV, the amount minted is uAmount = (vPrice * vAmount) / 1e18 where vPrice is the price of VADER in terms of USD.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/tokens/USDV.sol#L71-L76

When burning USDV, the amount of VADER released is vAmount = (uPrice * uAmount) / 1e18 where uPrice is the price of USDV in terms of USD.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/tokens/USDV.sol#L105-L109

We're therefore allowing people to claim VADER at an exchange rate of 1 VADER = 1 USD.

Now consider we do a mint and then burn the entire amount of USDV we receive, combining the two expressions above:

vAmountOut = (uPrice * (vPrice * vAmountIn) / 1e18 ) / 1e18
// Assume that uPrice ~= 1e18, i.e. USDV is at peg
vAmountOut = (vPrice * vAmountIn) / 1e18

It's then plain to see that if the price of VADER exceeds $1 at any point we can extract value from the system by minting and then burning USDV.

Recommended Mitigation Steps

Use the USDV:VADER exchange rate rather than USDV:USD when burning USDV for VADER.

Pay attention to the units implied by different values to ensure you're using them correctly.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Council can veto proposals to remove them to remain in power.

Proof of Concept

The Vader governance contract has the concept of a "council" which can unilaterally accept or reject a proposal. To prevent a malicious council preventing itself from being replaced by the token holders, the veto function checks the calldata for any proposal action directed at GovernorAlpha to see if it matches the changeCouncil function selector.

Note this is done by reading from the proposal.calldatas array.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L568-L603

If we look at the structure of a proposal however we can see that the function selector is held (in the form of the signature) in the signatures array rather than being included in the calldata. The calldata array then holds just the function arguments for the call rather than specifying which function to call.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L71-L72

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L356-L362

Indeed if we look at the TimeLock contract we see that the signature is hashed to calculate the function selector and is prepended onto the calldata.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/Timelock.sol#L292-L299

Looking at the function signature of the changeCouncil we can see that the value that the veto function will check against this.changeCouncil.signature will be the first 4 bytes of an abi encoded address and so will always be zero no matter what function is being called.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L623

High risk as this issue gives the council absolute control over the DAO such that they cannot be removed.

Recommended Mitigation Steps

Hash the function signatures to calculate function selectors and then check those rather than the calldata.

This is something that should be picked up by a test suite however, I'd recommend writing tests to ensure that protections you add to the code have any affect and more broadly check that the code behaves as expected.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Frontrunners can extract up to 100% of the value provided by LPs to VaderPoolV2 as fungible liquidity.

Proof of Concept

Users can provide liquidity to VaderPoolV2 through the mintFungible function.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex-v2/pool/VaderPoolV2.sol#L311-L317

This allows users to provide tokens in any ratio and the pool will calculate what fraction of the value in the pool this makes up and mint the corresponding amount of liquidity units as an ERC20.

However there's no way for users to specify the minimum number of liquidity units they will accept. As the number of liquidity units minted is calculated from the current reserves, this allows frontrunners to manipulate the pool's reserves in such a way that the LP receives fewer liquidity units than they should. e.g. LP provides a lot of nativeAsset but very little foreignAsset, the frontrunner can then sell a lot of nativeAsset to the pool to devalue it.

Once this is done the attacker returns the pool's reserves back to normal and pockets a fraction of the value which the LP meant to provide as liquidity.

Recommended Mitigation Steps

Add a user-specified minimum amount of LP tokens to mint.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Potential DOS on swaps on VaderPool

Proof of Concept

BasePool makes use of a validateGas modifier on swaps which checks that the user's gas price is below the value returned by _FAST_GAS_ORACLE.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex/pool/BasePool.sol#L292

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/dex/utils/GasThrottle.sol#L8-L22

Should _FAST_GAS_ORACLE be compromised to always return zero then all swaps will fail. There is no way to recover from this scenario.

Recommended Mitigation Steps

Either remove GasThrottle.sol entirely or allow governance to turn it off as is done in VaderPoolV2.sol

Handle

TomFrenchBlockchain

Vulnerability details

Impact

IL isn't properly converted from being in terms of USDV to VADER, resulting in reserve paying out incorrect amount.

Proof of Concept

VaderReserve.reimburseImpermanentLoss receives an amount in terms of USDV and converts this to an amount of VADER to send to recipient.

However as shown in the link if there is a previous price stored for USDV, the amount of VADER tokens to be sent to the recipient is amount / usdvPrice.

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/reserve/VaderReserve.sol#L84-L110

usdvPrice is the total USD value of foreign assets divided by the total amount of USDV in a number of pairs. It's then some measure of the inverse of the price of USDV in USD, nothing to do with converting into VADER.

The reserve will then improperly calculate the amount of VADER to pay out once there is a single reading of the USDV price.

Recommended Mitigation Steps

It looks like both branches of this if statement are supposed to be run, i.e. convert from USDV to USD and then to VADER but I can't be sure. Care should be taken so that the calculation being performed is the expected one.

Handle

TomFrenchBlockchain

Vulnerability details

I've put this as a medium issue as we're leaking value as users are stuck with assets which are likely to be worth much less as they are deprecated. It could also be low as it's not exploitable by outside parties and the loss isn't taken by the protocol but the user.

Impact

Potential for users to lose the right to convert VETH to VADER, being stuck with a deprecated token.

Proof of Concept

Should a user have a zero allowance of VETH on the converter, no VETH will be taken and no VADER will be paid out as L147 will set the amount to zero.

https://github.com/code-423n4/2021-12-vader/blob/28d3405447f0c3353964ca755a42562840d151c5/contracts/tokens/converter/Converter.sol#L145-L150

There is a minVader check on L153 to enforce a minimum output of VADER but should this be improperly set the transaction would succeed with the user receiving much less VADER than they expect.

Crucially, the merkle proof that was used in this transaction will be marked as invalid so the user will not be able to try again once they have set the proper allowance. Someone can then lose the opportunity to convert their VETH and are left with a worthless deprecated token if they are inattentive.

It seems like this is trying to handle the case where a user doesn't have the full amount of VETH as they are entitled to convert (by setting their allowance equal to their balance?). This is a pretty suboptimal way to go about this as it's extremely implicit so users are liable to make mistakes.

I'd recommend decoupling the merkle proof from conversion of VETH to VADER:

1. Change the merkle proof to set an amountEligibleToConvert value in storage for each user (which would be initially set to amount).
2. Allow a user to then convert VETH to VADER up to their amountEligibleToConvert value, subtracting the amount converted from this each time.

For gas efficiency we can use a sentinel value here to mark a user which has claimed their full quota already distinctly from someone who hasn't provided a merkle proof yet (to avoid having to track this separately in another storage slot)

These two steps could be chained in a single transaction to give a similar UX as currently but would also allow users to recover in the case of partial conversions.

Recommended Mitigation Steps

As above. I'd caution against just stating "The frontend will handle this correctly so this isn't an issue", there will be users who interact with the contract manually so it's important to make the interface safe where possible.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Loss of resilience of oracle to a faulty pricing for a single pair.

Proof of Concept

In the oracle we calculate the TVL of each pool by pulling the reserves and multiplying both assets by the result of a supposedly manipulation resistant oracle (the oracle queries its previous value for USDV and pulls the foreign asset from chainlink).

https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/lbt/LiquidityBasedTWAP.sol#L353-L383

This value can be manipulated by skewing the reserves of the underlying pair with a flashloan attack. An attacker can then make a pool appear with an arbitrarily large currentLiquidityEvaluation which will result in all other pairs contributing negligibly to the final result of the oracle.

This doesn't result in loss of funds by itself afaict but should there be an issue for the chainlink price feed for the asset in any pool then an attacker can force the oracle to only use that pool for pricing USDV/VADER

Medium risk as "Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements." External requirements being a malfunctioning or deprecated chainlink pricefeed for any used asset.

Calculating TVL of the pool is equivalent to value of all LP tokens so for more information see this post: https://blog.alphafinance.io/fair-lp-token-pricing/

Recommended Mitigation Steps

Calculate fair reserves using the pool invariant and the fair prices of the two assets.

The above link contains a mitigates for Uniswap, a similar calculation would have to be performed which is specific for the Vader invariant.