search

sherlock-audit / 2022-09-knox-judging

0 stars 0 forks source link

cccz - A malicious keeper can manipulate the LToken's pricePerShare to take an unfair share of future users' deposits #115

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

cccz

medium

A malicious keeper can manipulate the LToken's pricePerShare to take an unfair share of future users' deposits

Summary

A well known attack vector for almost all shares based liquidity pool contracts, where an early user(keeper) can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

Vulnerability Detail

Only the keeper can call the initializeEpoch function of the VaultAdmin contract to deposit asset tokens into the Vault. In the first epoch, a malicious keeper can 'deposit' with 1 wei of asset token as the first depositor of the Vault, and get 1 wei of shares. Then the keeper can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) . In later epochs, the shares gotten in processDeposits may be 0 due to loss of precision.

Impact

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Code Snippet

https://github.com/sherlock-audit/2022-09-knox/blob/main/knox-contracts/contracts/vault/VaultAdmin.sol#L234-L243 https://github.com/sherlock-audit/2022-09-knox/blob/main/knox-contracts/contracts/queue/Queue.sol#L202-L217 https://github.com/solidstate-network/solidstate-solidity/blob/master/contracts/token/ERC4626/base/ERC4626BaseInternal.sol#L219-L232 https://github.com/solidstate-network/solidstate-solidity/blob/master/contracts/token/ERC4626/base/ERC4626BaseInternal.sol#L142-L149 https://github.com/solidstate-network/solidstate-solidity/blob/master/contracts/token/ERC4626/base/ERC4626BaseInternal.sol#L41-L59 https://github.com/sherlock-audit/2022-09-knox/blob/main/knox-contracts/contracts/vault/VaultInternal.sol#L156-L163 https://github.com/sherlock-audit/2022-09-knox/blob/main/knox-contracts/contracts/vault/VaultInternal.sol#L102-L125

Tool used

Manual Review

Recommendation

Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L124

0xCourtney commented 1 year ago

@pbwaffles from my understanding this attack works as follows:

  1. Attacker deposits 1 wei of the vault asset, and receives 1 vault share in return. The PricePerShare is currently 1. Note, the first depositor will always set the initial PricePerShare.
  2. Attacker deposits a small amount of the vault asset (e.g. 1*10^6 * 1E18). This would severely inflate the PricePerShare from 1 to 1**24 ((1 + (1*10^6 * 1E18)) / 1 ).
  3. Victim deposits 2*10^6 * 1E18 however they will only receive 1 vault share.
  4. Vicitm and Attacker both hold a 50/50 share of the vault, which nets the Attacker a 0.5*10^6 * 1E18 vault assets.

What I don't understand fully is why the victim in step 3 would receive 1 vault share. I reference this audit in my example.

Evert0x commented 1 year ago

For example if we make the donation amount 5 times the size of the Victim deposit amount (donating 10*10^6 * 1E18 instead of 1*10^6 * 1E18)

shareAmount = (assetAmount * supply) / totalAssets; https://github.com/solidstate-network/solidstate-solidity/blob/master/contracts/token/ERC4626/base/ERC4626BaseInternal.sol#L56

In step 3 the 

>>> (2*10**6 * 1e18 * 1) / ((10*10**6 * 1e18) + 1)
0.19999999999999998

This will round down to 0 shares, giving the victim 0 shares for his deposit.

Assumptions are

0xCourtney commented 1 year ago

Thanks for the clarification @Evert0x. A few things:

  1. The Keeper is an EOA owned/controlled by the protocol team and therefore considered trusted.
  2. Only the Queue can deposit directly into the Vault. Deposits are batched and processed once per epoch. Everyone who deposited into the Queue during an epoch will receive the same price per share. Therefore the attacker would need to be the only one in the Queue for the first epoch.
arbitrary-CodeBeholder commented 1 year ago

Sorry for coming late to this line of inquiry.

I agree with you @0xCourtney , given that the Keeper is an account controlled by the protocol team it seems to be a non-issue.