`Synth` tokens can get over-minted

[code423n4]

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.

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