Chinmay - First depositors in a pool may lose their funds because of wrong shares calculation

[github-actions]

Chinmay

high

First depositors in a pool may lose their funds because of wrong shares calculation

Summary

In the deposit function, there is a call to function tokenToShares() to calculate shares based on amount, to be given to the depositor. This function has a protection against minting of less shares due to solidity rounding down, but this protection is not being used when making a call through the deposit function.

Vulnerability Detail

The next line of this tokenToShares() call at L#324 has a require(shares > 0) statement but this does not protect the depositor entirely.

Consider the following scenario :

1. User wants to deposit 200 wei of a token
2. Attacker sees this and frontruns user's transaction by depositing 1 wei (and gets 1 share) and then sending 100 wei directly to the contract.
3. The balanceOf(contract) gets counted in "_supplied" hence when finally the user's transaction gets executed, the calculation becomes :
   shares = 200*1/101 = 1.99 which gets rounded down to 1

Then the attacker withdraws his tokens and gets 150 wei because both user and attacker own 1 share. So a profit of 50 wei at the cost of the first user.

Impact

The first depositor will lose 25 % of his funds to the attacker.

Code Snippet

https://github.com/sherlock-audit/2023-02-surge/blob/main/surge-protocol-v1/src/Pool.sol#L324

Tool used

Manual Review

Recommendation

Actually there is a proper check in place to prevent this. ie. the bool roundUpCheck parameter in tokenToShares() function, but while calling it from deposit() the parameter is set to false.

If it is set to true, the shares would have been rounded up by 1 in the above mentioned situation and the first depositor would get 2 shares. Thus preventing the loss of user's funds.

Duplicate of #125