0xSmartContract - First depositor can steal asset tokens of others

[sherlock-admin2]

0xSmartContract

high

First depositor can steal asset tokens of others

Summary

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors.

Vulnerability Detail

v2-core-audit-2023-07-14/src/vault/LMPVault.sol:
  586       */
  587:     function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
  588:         uint256 supply = totalSupply();
  589: 
  590:         // slither-disable-next-line incorrect-equality
  591:         shares = (assets == 0 || supply == 0) ? assets : assets.mulDiv(supply, totalAssets(), rounding);
  592:     }

Impact

Take a look at this transmissions11/solmate#178

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L587C1-L592C6

Tool used

Manual Review

Recommendation

Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.

Ensure the number of shares to be minted is non-zero: require(shares != 0, "zero shares minted");

Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()

Call deposit() once in initialize() to achieve the same effect as the suggestion above.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid. totalAsset = totalIdle + totalDebt, totalAsset doesn't use balance of contract. Any deposit will increase totalIdle, so first depositor can't inflate shares of LMPVault