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-13 #113

Closed 0xKitsune closed 1 year ago

0xKitsune commented 1 year ago

Status

Reported

Type

Vulnerability

Severity

Highest

Code Snippet:

Remediation

Description


MevEth is an ERC4626 vault with the share rate calculated from fraction ( elastic and base ). Fraction is decreased in an unchecked manner in withdraw and redeem . In other words, it assumes that it cannot underflow because the shares are burned right after.

But because MevEth also implements OFT (LayerZero’s ERC20), it is possible to send tokens from another chain to this chain and obtain more shares than there are counter in fraction.base .

By consequently withdrawing or burning those shares, the fraction will underflow, either hugely inflating the share rate (where 1 share is worth the entire vault) or hugely deflating the share rate (where all shares are worth nothing), depending on whether the attacker chooses to underflow base or elastic .

In the former case, they would be able to steal all the funds from the contract.
sandybradley commented 1 year ago

I think this was a misunderstanding of how the OFT setup works. Related slack messages:

13 (Insufficient handling of OFT leads to underflow of elastic/base) All alt chain OFTs are initialized with 0 supply. Shares are only minted when the corresponding amount is burned on eth mainnet. This means that fraction.base should account for all deposits. Therefore any shares sent back to mainnet for withdrawl have already been accounted for.

Regarding issue 13, is it correct that on alt chains only an OFT is going to deployed and the main contract (MevEth) is only on Mainnet? i.e. depositing/minting would only be done Mainnet?

RE: 13 - Yes, exactly. MevEth is deployed on mainnet only; deposit/mint & withdraw/redeem are mainnet only functions. OFT's are deployed on alt chains, with the same name and linked for bridging via layerzero but act as simple ERC20's limited to transfers only, with initial supply = 0.

Issue 13: I see, that would indeed make the issue invalid, it won't be in the final report.

sandybradley commented 1 year ago

This has been left out of the final report.