VaderPoolV2.mintFungible exposes users to unlimited slippage

[code423n4]

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.

[SamSteinGG]

Given that the codebase attempts to implement the Thorchain rust code in a one-to-one fashion, findings that relate to the mathematical accuracy of the codebase will only be accepted in one of the following cases:

• The code deviates from the Thorchain implementation
• A test case is created that illustrates the problem

While intuition is a valid ground for novel implementations, we have re-implemented a battle-tested implementation in another language and as such it is considered secure by design unless proven otherwise.

An additional note on this point is that any behaviour that the Thorchain model applies is expected to be the intended design in our protocol as well.

An important example is the slippage a user incurs on joining a particular LP pool for which there is no check as there can't be any. Enforcing an LP unit based check here is meaningless given that LP units represent a share that greatly fluctuates (1 unit of LP out of 100 units is different than 1 out of 1000, however, a slippage check for 100 units of DAI for example is valid).