Vader Protocol contest - TomFrenchBlockchain's results

Handle

TomFrench

Vulnerability details

Impact

The amount of synths minted / assets received when minting or burning synths can be manipulated to an unlimited extent by manipulating the reserves of the pool

Proof of Concept

See VaderPool.mintSynth: https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/pool/VaderPoolV2.sol#L126-L167

Here a user sends nativeDeposit to the pool and the equivalent amount of foreignAsset is minted as a synth to be sent to the user. However the user can't specify the minimum amount of synth that they would accept. A frontrunner can then manipulate the reserves of the pool in order to make foreignAsset appear more valuable than it really is so the user receives synths which are worth much less than what nativeDeposit is worth. This is equivalent to a swap without a slippage limit.

Burning synths essentially runs the same process in behalf so manipulating the pool in the opposite direction will result in the user getting fewer of nativeAsset than they expect.

Recommended Mitigation Steps

Add a argument for the minimum amount of synths to mint or nativeAsset to receive.

Handle

TomFrench

Vulnerability details

Impact

Draining of funds from VaderPool

Proof of Concept

See the VaderPool.mintSynth function: https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/pool/VaderPoolV2.sol#L126-L167

As the pool's reserves can be manipulated through flashloans similar to on UniswapV2, 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

Prevent minting of synths or at the very least tie the exchange rate to a manipulation resistant oracle.

Handle

TomFrench

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L156

On L156 of TwapOracle we perform the calculation:

result = ((sumUSD * IERC20Metadata(token).decimals()) / sumNative);

This seems extremely odd as for an 18 decimal token we're then calculating

result = ((sumUSD * 18) / sumNative);

This is just plain weird. I expect what was meant is to replace this line with the below so we're properly scaling for token's number of decimals.

uint256 scalingFactor = 10 ** IERC20Metadata(token).decimals()
result = (sumUSD * scalingFactor) / sumNative;

Marked as high severity as this exchange rate appears to be used in some form of minting mechanism and correctness of the oracle is listed as one of the key focuses of the audit.

Recommended Mitigation Steps

As above.

Handle

TomFrench

Vulnerability details

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-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L237-L269

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

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/router/VaderRouterV2.sol#L208-L227

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-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/math/VaderMath.sol#L73-L93

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.

Recommended Mitigation Steps

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

Handle

TomFrench

Vulnerability details

Impact

Vader Reserve can be drained of funds.

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-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/pool/VaderPool.sol#L77-L89

These losses are then refunded to the LP in VADER tokens from the reserve. NOTE: This IL protection is paid for ALL token pairs. THIS IS IMPORTANT!

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/router/VaderRouter.sol#L187-L206

The loss is calculated by the comparing the amounts of each asset initially added to the pool against the amounts of each asset which are removed from the pool. There's an unspoken assumption here that the LP entered the pool at the true price at that point.

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

Crucially we see that if an attacker can cheaply create a pool with a token which starts off with a very low price in terms of VADER and is guaranteed to have a very high price in terms of VADER then they will benefit from a large amount of IL protection funds from the reserve.

An attacker could then perform this attack with the following.

1. Flashloan a huge amount of Vader (or flashloan + buy VADER).
2. Deploy a token TKN, which the attacker can mint as much as they like.
3. Add liquidity to a new pool with a large amount of VADER and a small amount of TKN
4. Use their ability to mint TKN to buy up all the VADER in their pool
5. Repay flashloan with VADER extracted from pool + some pre-existing funds as attacker needs to cover VADER lost to swap fees/slippage.

The attacker has now engineered a liquidity position which looks massively underwater due to IL but in reality was very cheap to produce. Nobody else can do anything to this pool except just give the attacker money by buying TKN so this attack can't be prevented. The attacker now just needs to wait for at most a year for the IL protection to tick up and then they can cash in the LP position for a nice payday of up to the amount of VADER they initially added to the pool.

Recommended Mitigation Steps

Add a whitelist to the pairs which qualify for IL protection.

Handle

TomFrench

Vulnerability details

Impact

All liquidity deployed to one of VaderPool or VaderPoolV2 will be locked permanently.

Proof of Concept

Both VaderRouter and VaderRouterV2 make calls to VaderReserve in order to pay out IL protection.

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/router/VaderRouter.sol#L206

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/router/VaderRouterV2.sol#L227

However VaderReserve only allows a single router to claim IL protection on behalf of users.

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/reserve/VaderReserve.sol#L80-L83

It's unlikely that the intent is to deploy multiple reserves so there's no way for both VaderRouter and VaderRouterV2 to pay out IL protection simultaneously.

This is a high severity issue as any LPs which are using the router which is not listed on VaderReserve will be unable to remove liquidity as the call to the reserve will revert. Vader governance is unable to update the allowed router on VaderReserve so all liquidity on either VaderPool or VaderPoolV2 will be locked permanently.

Recommended Mitigation Steps

Options:

1. Allow the reserve to whitelist multiple addresses to claim funds
2. Allow the call to the reserve to fail without reverting the entire transaction (probably want to make this optional for LPs)

Handle

TomFrench

Vulnerability details

Impact

The VaderReserve pays out IL from VaderPoolV2 LPs expressed in USDV with VADER (assuming a 1:1 exchange rate)

Proof of Concept

From the TwapOracle, it can be seen that VaderPoolV2 is intended to be deployed with USDV as its nativeAsset:

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L281-L296

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/BasePoolV2.sol#L58-L59

All the pairs in VaderPoolV2 are then USDV:TKN where TKN is some other token, exactly which is irrelevant in this case.

VaderPoolV2 offers IL protection where any IL is refunded from the VaderReserve

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L258-L268

The VaderReserve holds a balance of VADER tokens which will be used to pay out this protection.

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/reserve/VaderReserve.sol#L76-L90

The IL experienced by the LP is calculated in VaderMath.calculateLoss

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

This is the core of the issue. From the variable names it's clear that this is written with the assumption that it is work on units of VADER whereas it is provided amounts in terms of USDV. Checking VaderRouterV2 we can see that we pass the output of this calculation directly to the reserve in order to claim VADER.

If an LP experienced 100 USDV worth of IL, instead of claiming the equivalent amount of VADER they would receive exactly 100 VADER as there's no handling of the exchange rate between USDV and VADER.

As VADER and USDV are very unlikely to trade at parity LPs could get sustantially more or less than the amount of IL they experienced.

Recommended Mitigation Steps

Add handling for the conversion rate between VADER and USDV using a tamper resistant oracle (TwapOracle could potentially fulfil this role).

Handle

TomFrench

Vulnerability details

Impact

Total loss of funds which have been approved on VaderPoolV2

Proof of Concept

VaderPoolV2 allows minting of fungible LP tokens with the mintFungible function

https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/pool/VaderPoolV2.sol#L284-L290

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

Recommended Mitigation Steps

Remove from argument and use msg.sender instead.

Handle

TomFrench

Vulnerability details

Impact

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

Proof of Concept

Users can provide liquidity to VaderPoolV2 through the mintFungible function.

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

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 liqudity.

Recommended Mitigation Steps

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

Handle

TomFrench

Vulnerability details

Impact

LPs are subject to incurring unlimited slippage due to manipulation of the pool's reserves.

Proof of Concept

If we look at VaderRouterV2 we can see that the arguments amountAMin and amountBMin of the addLiquidity function are unused.

https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/router/VaderRouterV2.sol#L77-L96

We can compare this with Uniswap V2's router to see how these parameters are used

https://github.com/Uniswap/v2-periphery/blob/dda62473e2da448bc9cb8f4514dadda4aeede5f4/contracts/UniswapV2Router02.sol#L33-L60

Uniswap uses the current reserves to check the equivalent value of amountADesired in terms of tokenB and then crucially checks that amountBOptimal >= amountBMin. This is important as a frontrunner can manipulate the reserves of the pool right before the LP joins. By placing a minimum on the amount of tokenB that is being contributed it places bounds on the relative values of the two tokens in the pool at the point at which the LP joins - otherwise an attacker could make tokenB appear unrealistically valuable at the point at which an LP is joining with a lot of tokenA.

However in Vader Protocol no such slippage protection exists. Whatever tokens the LP provides are added to the pool in whatever ratio as it doesn't require equal values of each token as Uniswap does.

In this situation an LP may supply a large amount of tokenA and zero tokenB, while an sandwicher frontruns them to sell a huge amount of tokenA to the pool to tank its value. tokenA is now treated as near worthless and the pool behaves as if the LP only provided a very small amount of value to the pool. The sandwicher can then backrun the LP addition to extract much of this value which should have belonged to the LP.

Recommended Mitigation Steps

Reinstate these arguments to ensure that the pools relative reserves are within some user specified bounds at the point of joining.

Handle

TomFrench

Vulnerability details

Impact

Loss of ability of TwapOracle to update should too many pools be added.

Proof of Concept

TwapOracle allows an unlimited number of pairs to be added and has no way of removing pairs after the fact. At the same time TwapOracle.update iterates through all pairs in order to update value for each pair.

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L322-L369

TwapOracle.registerPair is a permissioned function so that only the owner can add new pairs however should the owner account be compromised or not mindful of the number of pairs being added it is possible to put the oracle into a state in which it is unable to update. The oracle cannot recover from this state.

Recommended Mitigation Steps

Possible options:

• Add a method to stop tracking a particular pair
• Allow updating a subset of all pairs at a time.

Handle

TomFrench

Vulnerability details

Impact

Inability to call consult on the TwapOracle and so calculate the exchange rate between USDV and VADER.

Proof of Concept

Should any of the Chainlink aggregators used by the TwapOracle becomes stuck in such a state that the check on L143-146 of TwapOracle.sol consistently fails (through a botched upgrade, etc.) then the consult function will always revert.

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L143-L146

There is no method to update the address of the aggregator to use so the TwapOracle will be irrecoverable.

Recommended Mitigation Steps

Allow governance to update the aggregator for a pair (ideally with a timelock.)

Handle

TomFrench

Vulnerability details

Impact

Potentially frozen or purposefully inaccurate USDV:VADER price feed.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L322

Only the owner of TwapOracle can call update on the oracle. Should the owner desire they could cease calling update on the oracle for a period. Over this period the relative prices of VADER and USDC will vary.

After some period timeElapsed the owner can call update again. A TWAP is a lagging indicator and due to the owner ceasing to update the oracle so timeElapsed will be very large, therefore we're averaging over a long period into the past resulting in a value which may not be representative of the current USDV:VADER exchange rate.

The owner can therefore selectively update the oracle so to result in prices which allow them to extract value from the system.

Recommended Mitigation Steps

Remove the permissioning from TwapOracle.update

Handle

TomFrench

Vulnerability details

Impact

Any unaccounted for tokens on VaderPoolV2 can be siphoned off by anyone

Proof of Concept

VaderPoolV2 has a rescue function which allows any unaccounted for tokens to be recovered.

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

However there is no access control on this function which means than should any tokens be sent to VaderPoolV2 by accident they'll just be scooped up by flashbots rather than being recoverable by the original owner or Vader governance.

This also means that any rebasing tokens which are deposited into VaderPoolV2 will have any rebases lost rather than being recoverable by Vader governance.

Recommended Mitigation Steps

Permission this function to only allow Vader governance to claim tokens.

Handle

TomFrench

Vulnerability details

Impact

Potential DOS on swaps

Proof of Concept

BasePool and BasePoolV2 make 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-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex/utils/GasThrottle.sol#L9-L20

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