Audit: MANETH-19

[sandybradley]

GRANTING REMAINING REWARDS AND FEES SHOULD BE ENFORCED BY CODE WHEN UPDATING

SEVERITY: High

PATH: MevEth.sol:finalizeUpdateMevEthShareVault():L253-274

https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/MevEth.sol#L253-L274

DESCRIPTION

The function finalizeUpdateMevEthShareVault is used to update the share vault contract. In the function, when updating mevEthShareVault = pendingMevEthShareVault on line 269,

there is no check to see if protocolBalance.rewards and protocolBalance.fees equal zero.

If the new mevEthShareVault overwrites these variables, there will be a permanent imbalance when the operators tracking RewardPayment events calculate protocolFeesOwed, leading to protocol malfunction and potential loss of user funds.

In addition, granting any remaining rewards from the existing share vault as suggested in MevEth.sol line 264 is not enough. Consider the following scenarios:

Scenario 1:

• commitUpdateMevEthShareVault() is called
• Pre-update mevEthShareVault info: address(this).balance = 0, protocolBalance.rewards = 0, protocolBalance.fees = 0.
• receive is called (an arbitrary number of times) and RewardPayment is emitted, now address(this).balance > 0, rewards and fees are still zero.
• mevEthShareVault update is finalised via finalizeUpdateMevEthShareVault()
• New mevEthShareVault: address(this).balance = 0, however operators are calculating protocolFeesOwed based on last RewardPayment emitted.
• Now logRewards will always revert on line 102 because protocolFeesOwed > uint128(rewardsEarned). Eventually enough payments will accumulate and logRewards won’t revert, however as soon as that happens it is expected that operators will call payRewards(), quickly putting the contract in debt again.

if (protocolFeesOwed > uint128(rewardsEarned)) {
    revert FeesTooHigh();
}

• This will make any function that relies on fraction.elastic stale, for example making users having to wait for indefinite periods of time to redeem assets they are entitled to.

Scenario 2:

In the case rewards and fees were not granted before the update.

• Everything that happened in scenario one + un-granted rewards and fees are lost.
• In this scenarios users would never be able to claim all of their assets. To prevent such scenarios checks before updates should be enforced in code.

REMEDIATION

• implement checks in finalizeUpdateMevEthShareVault() to ensure rewards and fees equal zero before setting mevEthShareVault = pendingMevEthShareVault
• implement a function to recover any remaining RewardPayments to the new mevEthShareVault so there are no discrepancies between balances

[0xKitsune]

We could implement something like the following, however I recall being informed that the initial share vault will be a multisig. With this in mind, we could not use the following logic to get the balance of the protocol fees and rewards unless we use SafeModules or something similar.

    function finalizeUpdateMevEthShareVault() external onlyAdmin {
       // --snip--
        (uint128 fees, uint128 rewards) = MevEthShareVault(payable(mevEthShareVault)).protocolBalance();

        if (fees != 0 || rewards != 0) {
            revert MevEthErrors.NonZeroVaultBalance();
        }
       // --snip--
    }

Is the plan to still use a multisig as the initial share vault? If so, what are your thoughts on how to approach this? @ControlCplusControlV @sandybradley

[ControlCplusControlV]

@0xKitsune that is still the plan, but we could always default to the .balance of the multisig if the call fails using a sort of try catch maybe?

[sandybradley]

Second remediation point is covered in https://github.com/manifoldfinance/mevETH2/pull/157 where grantRewards is not restricted to only current share-vault.

[sandybradley]

@0xKitsune that is still the plan, but we could always default to the .balance of the multisig if the call fails using a sort of try catch maybe?

Issue with using balance for multisig is that its assuming all the eth balance is from staking rewards. Surely there would at least be some excess for paying gas.

[0xKitsune]

After talking with @ControlCplusControlV I opened #165 as a first pass at this.

With the PRed approach, the function to finalize the new MevEthShareVault takes a bool signifying if the previous share vault is a multisig or not.

In the case that the previous share vault is not a mutisig, it will require that the protocol rewards and balance are 0 before upgrading the module. If the previous vault is a multisig, then this condition is not checked and the multisig is responsible for forwarding the necessary balances.

As mentioned before, when the share vault is a multisig, we could check that the balance of the address is 0, which would require the multisig to migrate fees to another address upon upgrading, but it would also ensure that funds have been migrated before finalization.

@sandybradley @ControlCplusControlV, let me know your thoughts and we can iterate from here.