first user in the system can steal all incoming assets

[code423n4]

Handle

egjlmn1

Vulnerability details

In Vault.sol https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol when you deposit tokens using function deposit(DepositParams calldata _params) external it calculates your shares using the ratio of the totalShares to the totalUnderlyingToken. Lets see the following attack:

1. the attacker join the system for the first time with an amount 1. now the totalShares = totalUnderlying = 1
2. the attacker sees a user is about to deposit X tokens
3. the attacker frontrun before the user and ERC20.transfer the underlying token to the system the same amount, X.
4. when the user's transaction will happen, the totalShares is equal to 1 and the totalUnderlying is equal to X+1, resulting in the share calculation of the user depositing X to be 0. (X * 1 / (X+1) = 0)
5. not only that the user got 0 shares and he paid the protocol X tokens, the attacker can now withdraw his money and get both his deposit and the user's deposit.

As you can see, if a malicious user enter the system first, he can steal all the tokens from all the other users who try to deposit after him (given he has more money than them)

Notice that even the tokens the attacker transferred directly to the system using ERC20.transfer is not lost because the amount to withdraw is calculated using the ERC20.balanceOf(address(this))

Impact

An attacker can successfully steal every deposit sent to the system given he has more money than the deposited amount (he can always call withdraw to increase his balance from the previous stolen assets if he doesn't have enough to steal from the current deposit)

Proof of Concept

See the steps I wrote at the beginning

Tools Used

Manual code review

Recommended Mitigation Steps

Like Uniswap, the first user who deposits tokens should only be able to deposit a big amount (e.g. 1e18) and will not be able to withdraw.

[naps62]

I'm not sure this is a real issue. We're applying a multiplier of 10e18 to the share calculation. So after an initial deposit of 1, totalShares will actually be 10e18 An ERC20 transfer to the vault is just equivalent to generating yield for it. Future deposits will get less shares, but with a correct calculation, and not 0

(we need to test this to confirm)

cc/ @ryuheimat @gabrielpoca

[r2moon]

I didn't noticed about SHARES_MULTIPLIER. I think @naps62 you are right. not an issue, i think.

[naps62]

Disproven in the PR linked above

[dmvt]

Per sponsor's PR. Invalid.