manifoldfinance / mevETH2

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

feat: include cream balance in setup to simplify redeemCream and crea… #253

Closed sandybradley closed 11 months ago

sandybradley commented 11 months ago

…m rewards

okkothejawa commented 11 months ago

LGTM pending above fix, should make deployment much smoother, but pinging @okkothejawa for his thoughts on this. It's not quite a remediation but curious his thoughts on doing migration like this right away

I need to think further about the accounting consequences of this but this changes makes the first branches of convert functions redundant as fraction.base and fraction.elastic can never be 0. Similarly it makes withdrawQueue's fee exception for the last withdrawer redundant for the same reason. If I am not mistaken, as shares are not minted during the constructor for these fraction.base they can't be burned either so fraction.base or fraction.elastic has an effective zero point of these ether values. Which I don't like, as I would expect the fraction.base to reflect the amount of shares minted up to date.

sandybradley commented 11 months ago

To clear up motivation behind this PR:

In answer to the issues raised, faction base and elastic can be zero if all CrEth2 is redeemed for mevETH and withdrawn, fraction elastic and base will be zero. This then applies to the withdraw fee exception too. So this number represent pre-minted shares.

ControlCplusControlV commented 11 months ago

@sandybradley alright so after discussion with @okkothejawa I am planning on reverting this PR for 2 reasons

1) It effectively accrues rewards for crETH2 holders which is against spec 2) This creates a discrepancy that isn't effectively fixed until everyone migrates over creETH, as the ratio of elastic and base implicitly includes some amount of crETH2 holders

sandybradley commented 11 months ago

I contest these points but concede to the majority decision. I would argue the reverse is true.

okkothejawa commented 11 months ago

I contest these points but concede to the majority decision. I would argue the reverse is true.

  • CrEth2 holders are entirely responsible for current rewards being accrued
  • Pre-accounting at a 1:1 ratio gives CrEth2 holders no more advantage than accounting later at a different rate
  • Not accounting for current validators upfront creates a discrepancy that requires everyone from CrEth2 migrate over before rewards can be paid

"Not accounting for current validators upfront creates a discrepancy that requires everyone from CrEth2 migrate over before rewards can be paid"

Can you elaborate on this?

sandybradley commented 11 months ago

I'm confident we can mange reward payments to mitigate this but for completeness, my main issue revolves around the rewards payments.

sandybradley commented 11 months ago

As an alternative to solve for 2nd issue would be to fix the elastic increase to be 1:1 with base in redeemCream i.e. change: https://github.com/manifoldfinance/mevETH2/blob/2638ca46ed1c3460bc662bb3a9cbb2b44cca25aa/src/MevEth.sol#L755-L760

to

        // Update the fraction elastic and base
        if (mevEthAmount < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall();

        fraction.elastic += uint128(mevEthAmount);
        fraction.base += uint128(mevEthAmount);
sandybradley commented 11 months ago

@ControlCplusControlV @okkothejawa thoughts?

ControlCplusControlV commented 11 months ago

As an alternative to solve for 2nd issue would be to fix the elastic increase to be 1:1 with base in redeemCream i.e. change:

https://github.com/manifoldfinance/mevETH2/blob/2638ca46ed1c3460bc662bb3a9cbb2b44cca25aa/src/MevEth.sol#L755-L760

to

        // Update the fraction elastic and base
        if (mevEthAmount < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall();

        fraction.elastic += uint128(mevEthAmount);
        fraction.base += uint128(mevEthAmount);

Hmm, what the consequences of this though? Doesn't it effectively allow all crEth2 to be redeemed as if it was at the genesis? We ideally want to encourage people to migrate quickly and only allowing rewards in doing so is a good way to do that

sandybradley commented 11 months ago

Doesn't it effectively allow all crEth2 to be redeemed as if it was at the genesis?

sandybradley commented 11 months ago

Another alternative is to mint shares at construction to transfer at redemption i.e. in constructor:

        // set initial balance of validators
        fraction.elastic = uint128(28_448 ether);
        fraction.base = uint128(28_448 ether);
        _mint(address(this), 28_448 ether);

in redeemCream:

function redeemCream(uint256 creamAmount) external {
        _stakingUnpaused();
        if (creamAmount == 0) revert MevEthErrors.ZeroValue();

        // Calculate the equivalent mevETH to be redeemed based on the ratio
        uint256 mevEthAmount = creamAmount * uint256(CREAM_TO_MEV_ETH_PERCENT) / 1000;

        if (mevEthAmount < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall();

        // Burn CreamEth2 tokens
        IERC20Burnable(creamToken).burnFrom(msg.sender, creamAmount);

        // Transfer mevETH
        ERC20(address(this)).safeTransfer(msg.sender, mevEthAmount);

        // Emit event
        emit CreamRedeemed(msg.sender, creamAmount, mevEthAmount);
    }