crimson-rat-reach - [HIGH] Insurance Fund#depositFor - Insurance Funds can be manipulated and users can end up with 0 shares and permanent fund loss

[sherlock-admin]

crimson-rat-reach

high

[HIGH] Insurance Fund#depositFor - Insurance Funds can be manipulated and users can end up with 0 shares and permanent fund loss

Summary

A malicious user can manipulate the insurance funds so that subsequent users get 0 shares even after transferring funds.

Vulnerability Detail

The depositFor() function in InsuranceFund.sol decides on the number of shares that a depositor should have in the following way:

Impact

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

The value of _pool is determined by calling the_totalPoolValue() function, which iterates across all assets in the insurance fund pool and returns the sum of all assets. The _totalPoolValue() function determines the number of tokens of each asset in the following manner:

for (uint i; i < assets.length; i++) {
uint _balance = IERC20(address(assets[i].token)).balanceOf(address(this));

if (_balance == 0) continue;
uint numerator = _balance * uint(oracle.getUnderlyingPrice(address(assets[i].token)));
uint denomDecimals = assets[i].decimals;
totalBalance += (numerator / 10 ** denomDecimals);
}

This opens up a vulnerability since it uses the balanceOf()function that the IERC20 interface provides.

When the first user ever to interact with the insurance fund contract is about to deposit an ERC20 token, the attacker can front-run the transaction and simply transfer a small amount of an asset that the insurance fund supports apart from VUSD. Since depositFor()is not called here, shares are not issued, but the _pool value will be positive. Therefore, this statement will execute:

else {
shares = amount * _totalSupply / _pool;
}

Since _totalSupply is 0 and _pool is non-zero, the innocent user will get 0 shares, and subsequently, all users will start getting 0 shares.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is advised to keep track of assets deposited in the contract state itself so that ERC20 transfers which do not call the depositFor() function of the contract cannot manipulate the _pool value, or minimum liquidity must be added by the team and it must be added to the contract logic so that the team’s deposit is not front-run.

Duplicate of #140

[devblixt]

Escalate I think issue H-1 and this issue have the same root-cause and impact. Therefore, I believe this issue has a 'High' severity.

[sherlock-admin2]

Escalate I think issue H-1 and this issue have the same root-cause and impact. Therefore, I believe this issue has a 'High' severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ctf-sec]

See comments in https://github.com/sherlock-audit/2023-04-hubble-exchange-judging/issues/72

[0xvangrim]

See comments in #72

Thanks for this! We believe that this issue should be a duplicate of the following high vulnerability though: https://github.com/sherlock-audit/2023-04-hubble-exchange-judging/issues/140

Note the snippet here where we are describing the same vulnerability as the aforementioned issue:

When the first user ever to interact with the insurance fund contract is about to deposit an ERC20 token, the attacker can front-run the transaction and simply transfer a small amount of an asset that the insurance fund supports apart from VUSD. Since depositFor() is not called here, shares are not issued, but the _pool value will be positive. Therefore, this statement will execute:

else {
shares = amount * _totalSupply / _pool;
}

Since _totalSupply is 0 and _pool is non-zero, the innocent user will get 0 shares, and subsequently, all users will start getting 0 shares.

[hrishibhat]

Result: Low Duplicate of #140

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• devblixt: accepted