search

manifoldfinance / mevETH2

mevETH LST Protocol - Repo has migrated see link
https://github.com/MEV-Protocol/meveth
27 stars 2 forks source link

Audit: MANETH-48 #131

Closed sandybradley closed 1 year ago

sandybradley commented 1 year ago

LACK OF SLIPPAGE PROTECTION MIGHT CAUSE USERS UNEXPECTED LOSSES

SEVERITY: Medium

PATH: MevEth.sol:deposit, mint (L501-519, L543-560)

https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/MevEth.sol#L501-L519 https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/MevEth.sol#L543-L560

REMEDIATION:

introduce slippage control in deposit and mint so users can be certain that they will receive their expected amount of assets or shares, either when assets are moving in or out of the protocol for example, for deposit there should be a minShares parameter against which the resulting amount of share is checked. For mint there should be a maxAssets parameter against which the needed amount of assets is checked

DESCRIPTION

The functions deposit and mint allow a user to enter the protocol by depositing ETH for shares. deposit takes a specified amount of assets and mint takes a specified amount of shares. However neither function has slippage protection. For example, Alice might be expecting to receive 10 shares after calling deposit or mint in MevEth, however if Bob front-runs Alice with an action that changes the share rate, she will get less then what she desired, potentially causing unexpected loss of funds.

Step-by-step: ● Assume fraction.elastic = fraction.base = 100 ether.

  1. Alice calls previewDeposit (L482) and sees that with 10 assets she can get 10 shares.
  2. grantRewards is called and executed before Alices transaction, causing an increase in fraction.elastic of 10 ETH (total = 110 ETH), leading to convertToShares returning a value of 9.09 shares instead of 10 as desired. a. It is important to highlight that an attacker can front-run the operator calling payRewards in WagyuStaker with a direct ETH deposit so that the calculation in WagyuStaker line 91 increases fraction.elastic by a very significant amount with no upper boundary, for example 1000 ETH. This would lead to Alice only receiving 0.9 shares instead of 10 as expected. The steps highlighted above are also valid when using mint/previewMint instead of deposit/previewDeposit and would lead to more assets being transferred instead of less shares rewarded. This is specially dangerous if Alice has given unlimited WETH approval to MevEth. Slippage protection would also add an additional layer of protection in the case that a potential vulnerability that could manipulate the share exists. If Alice knew she would have received way less than expected she could have decided to wait until a later period when she could get a more desirable conversion rate.
sandybradley commented 1 year ago

I'm not convinced this finding is accurate. Though highlighting again payRewards call vulnerability in the mempool could use some attention like not letting the same user deposit and withdraw within a block ...

As for the finding details:

sandybradley commented 1 year ago

Related to https://github.com/manifoldfinance/mevETH2/issues/118

sambacha commented 1 year ago
  • payRewards can only be front-run if the operator uses the public mempool. This highlights the need for an operating standard, using only mev-relay execution to avoid this off the bat.

We would be incentivized only to include this on slots we have for ourselves anyway, as in a sense we are paying ourselves the execution cost vs other validators getting that fee

`