carrotsmuggler - Incorrect accounting in Insurance fund

[sherlock-admin]

carrotsmuggler

high

Incorrect accounting in Insurance fund

Summary

Incorrect accounting in InsuranceFund.sol contract can lead to easy loss of funds.

Vulnerability Detail

The contract InsuranceFund.sol stores user tokens in case of protocol insolvency. This contract however is susceptible to attacks via external transfers due to a bug in the code. This is different from the typical inflation attack scenario, which only happens if the contract is empty.

When a user is depositing some funds, the shares minted are calculated using the following expression:

if (_pool == 0) {
    shares = amount;
} else {
    shares = amount * _totalSupply / _pool;
}

So for a normal user, the protocol will use the else statement logic if funds are already in the contract from other users. Here _pool is calculated with the following logic.

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/InsuranceFund.sol#L303-L315

Thus the _pool value is not the total VUSD holdings. In fact, it is the total holdings of all tokens in the collateral array. Let's assume other than VUSD, ETH is also a valid collateral, and sending ETH to the contract also increases the _pool value.

During withdrawals, however, the logic is slightly different.

amount = balance() * shares / totalSupply();

Here balance() is the VUSD balance of the contract.

Lets assume the totalSupply is TS. The user deposits A no of tokens, netting them $A * TS / pool$ shares. Let's call that s. During withdrawal, the user gets s * balance() / TS tokens, where balance and TS have been updated due to the deposit of VUSD and mint. Thus for a mint and immediate withdrawal, the user payout is expected to be A. However, it actually is:

$$(A*TS/pool) * balance / TS_n$$

$$=(ATS/pool) * balance / (TS + ATS/pool)$$

$$=ATSbalance / (poolTS + ATS)$$

$$=A*balance/(pool + A)$$

Thus only if balance = pool + A, the user would get back their initial deposit. But balance only counts VUSD tokens, while pool counts all collateral tokens. If both counted only vusd tokens, then the balance would have been pool + A, but this is not the case here. Thus if there is any external deposit of ETH, such that balance != pool+A, then the depositor will lose funds, since pool is the larger value and is in the denominator.

Example:

A = 10e6
balance = 100e6 VUSD tokens in the contract
pool = 100e6 VUSD tokens + 100e6 usd worth of ETH in contract form external transfers, and ETH is a valid collateral.

Then the user gets only 5e6 tokens back, losing half their investment.

Impact

Users can lose funds due to the collateral being valued the same.

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/InsuranceFund.sol#L303-L315

Tool used

Manual Review

Recommendation

Calculation of balance and _pool should be done on the same tokens. Both should be done on VUDS only.

Duplicate of #72