Introduction & Scope

This audit looks into the following contracts of mevETH2: WagyuStaker.sol, MevEthShareVault.sol and MevEth.sol.

This audit was conducted by kebabsec members sai, FlameHorizon, okkothejawa and shung, with elcid as our new intern.

Note: This report does not provide any guarantee or warranty of security for the project.

Executive Summary

Table of Contents

Findings:

1. [INFORMATIONAL] Mismatch with access control

#5

Context: WagyuStaker.sol#L124-L125

Severity: Informational

Description: The comment in the lines above claims that the function is only callable by an admin, which does not happen, instead , the function is only callable by an operator, through the use of onlyOperator. This could be dangerous if the protocol wants to retain control of this function.

Recommendation: Change onlyOperator modifier for an onlyAdmin modifier, or equivalent to such.

2. [INFORMATIONAL] i++ Can be unchecked for gas savings

#6

Context: WagyuStaker.sol#L155-158

Severity: Informational

Description: In this situation, ++i can be unchecked as overflow is practically impossible.

Recommendation: Possibly uncheck to save gas

3. [INFORMATIONAL] dataLocation optimization

#7

Context: WagyuStaker.sol#L159

Severity: Informational

Description: The location of data would be better if it was calldata rather than memory, as it would save gas.

Recommendation: Possibly change memory for data

4. [INFORMATIONAL] Removed feature still referred in the comments

#8

Context: WagyuStake.sol#L86-L87

Severity: Informational

Description: In these lines, the comment block refers to a beneficiary having a role in the following function, but this isn’t defined anywhere in the contract, it seems like the feature was removed.

Recommendation: Possibly change the comment to stay in context with the function.

5. [LOW] Missing address check in constructor of MevEthShareVault.sol

#10

Context: MevEthShareVault.sol#L52-L55

Severity: Low

Description: The input variable _mevEth is not checked for zero address.

Recommendation: Add a zero address check to avoid errors.

6. [MEDIUM] send()Has hardcoded amount of gas it allows to use

#11

Context: MevEthShareVault.sol#L91-L94

Severity: low

Description: send() only allows the recipient contract to use 2300 gas. A smart contract functionality cannot be hard dependent on gas because the gas cost of some Opcodes are subject to change. Future price updates of opcodes can affect the viability of .send().

Recommendation: .call() should be used instead. Alternatively, Solmate’s safeTransferETH can also be used.

7. [INFORMATIONAL] Unnecessary variable logging in RewardPayment

#14

Context: MevEthShareVault.sol#L31-L34

Severity: Informational

Description: It is unnecessary to include block.number and block.coinbase within the event, as these can be retrieved easily from the block event was emitted from.

Recommendation: Instead, retrieve these from the block when the event is emitted.

8. [LOW] Events should be emitted for change in variables mevEth and protocolFeeTo

#15

Context: MevEthShareVault.sol#L59-L60

Severity: Low

Description: Variables mevEth and protocolFeeTo should be logged with events to keep track of changes.

Recommendation: Create an event to log changes in these two variables.

9. [INFORMATIONAL] Redundant subtraction in logRewards

#17

Context: MevEthShareVault.soll#L128-L133

Severity: Low

Description: In line 133, the event RewardsCollected does the subtraction rewardsEarned - protocolFeesOwed, this is uncessary, as this calculation is already done in line 128 to assign the variable _rewards.

Recommendation: use _rewards in the event instead of rewardsEarned - protocolFeesOwed.

10. [INFORMATIONAL] Redundant casting when assigning stakingModule

#19

Context: MevEth.sol#L207-210

Severity: Low

Description: In these lines, the castings are redundant, as pendingStakingModule is already declared as of type IStakingModule.

Recommendation: just using stakingModule = pendingStakingModule; should have the intended effect.

11. [LOW] Missing zero address check for newMevEthShareVault

#20

Context: MevEth.sol#L245-L257

Severity: Low

Description: It is perhaps better to have a zero address check for the variable newMevEthShareVault in commitUpdateMevEthShareVault in order to prevent time and gas waste in case of a mistake, as it currently finishes execution normally and the error only gets caught in the next step of the process in line 256.

Recommendation: Add zero address check for newMevEthShareVault

12. [LOW] Withdraw event does not account for fees

#21

Context: MevEth.sol#L610-L613

Severity: Low

Description: Line 613 emits Withdraw, but this does not take fees into account. convertToShares(assets) != shares in this context as withdraw and redeem passes down fee adjusted values for these variables.

Recommendation: Account for the fees in this event.

13. [MEDIUM]redeemCream Allows for minting of mevETH even when contract is paused

#22

Context: MevEth.sol#L721-L724

Severity: Medium

Description: While both deposit and mint calls _stakingUnpaused(), redeemCream doesn’t, meaning that there would be a way to mint mevETH through Cream ether even if the contract was paused. This may create risk during an emergency scenario of an ongoing hack concerning mevETH followed by a pause in this contract so that to prevent minting more mevETH, but this function would theoretically enable the minting of more mevETH.

Recommendation: Change _stakingUnpaused() so it also pauses minting, and use it in this function.

14. [LOW] _isZero Not more efficient than equality check

#25

Context: File.sol#L123

Severity: Low

Description: Using a separate function introduces a jump opcode, which is more costly than an equality check. Compiler optimization might also make a == 0 even more efficient.

Recommendation: Simply use an equality check

15. [LOW] Using type uint256for all constants is more efficient

#26

Context: MevEth.sol#L48-L59

Severity: Low

Description: Not using type uint256 for these variables incurs overhead fees when using them. Constants are not packed with regular variables so there is no gas saving here.

Recommendation: Use type uint256 for all constants.

16. [LOW] allowance Should not be decreased if a user has max allowance

#28

Context: MevEth.sol#L621-L626

Severity: Low

Description: Granting an entity max allowance should have the effect of virtually infinite spending, therefore should not be decreased in that scenario.

Recommendation: Do not reduce allowance in case of max allowance.

17. [MEDIUM] redeemCream function is not working as creamToken cannot be transferred to address(0)

#29

Context: MevEth.sol#L729-L732

Severity: Medium

Description: creamToken on mainnet reverts on transfer to address(0) therefore the function redeemCream does not work at all and it always reverts.

This can be seen here in the ERC20 contract of creamToken:

1
2
3
4
5
6
7
8

    function _transfer(
        address sender,
        address recipient,
        uint256 amount
    ) internal virtual {
        require(sender != address(0), "ERC20: transfer from the zero address");
        require(recipient != address(0), "ERC20: transfer to the zero address");
        ...

Recommendation: Instead, you could add

1
2
3

interface ERC20Burnable {
    function burnFrom(address account, uint256 amount) external;
}

and then use

1

 ERC20Burnable(address(creamToken)).burnFrom(msg.sender, creamAmount);

in the function to achieve the same effect of transfer to address(0).

18. [HIGH] redeemCream is vulnerable to a sandwich inflation attack

#30

Context: MevEth.sol#L738-L741

Severity: High

Description: This function leads to a high severity sandwiching the initial depositor to steal funds attack, see the PoC for it here. This attack depends on redeemCream working correctly (refer to #29), so for the PoC to work, transfer to zero address bug is patched by burning the tokens instead.

The issue arises as redeemCream does not implement the security measure of a minimum deposit, which prevents this attack to happen through other paths such as deposit succeeded by withdraw as _deposit asserts that the deposited amount of assets is no less than 0.01 ether. As this guarantees any deposit to be larger than 0.01 ether which makes truncation of the share amount to 0 virtually impossible. The following explains why this is almost impossible for the paths other than the redeemCream for mainnet and it also explains the attack more in detail:

1
2
3
4
5
6
7
8
9

function convertToShares(uint256 assets) public view returns (uint256 shares) {
        // So if there are no shares, then they will mint 1:1 with assets
        // Otherwise, shares will mint proportional to the amount of assets
        if (_isZero(uint256(fraction.elastic)) || _isZero(uint256(fraction.base))) {
            shares = assets;
        } else {
            shares = (assets * uint256(fraction.base)) / uint256(fraction.elastic);
        }
    }

This attack is facilitated by tricking convertToShares to return a 0 amount of shares even though a non-zero amount of assets is given to the function. This only happens when both fraction.base and fraction.elastic is non-zero and assets * fraction.base is smaller than fraction.elastic. Through redeemCream it is possible to make an initial deposit of 1 wei as minimum deposit check is not utilized, and as fraction.elastic can be inflated by directly dumping ether through grantRewards it is easily possible to get into a scenario of assets * fraction.base < fraction.elastic assuming assets is known before as this is a front-running attack.

The attacker can see the first legit deposit incoming, and can front-run it by the deposit of 1 wei with redeemCream, bringing both fraction.elastic and fraction.base to 1 wei. Then the attacker can inflate fraction.elastic to assets amount of the first legit deposit plus 1 wei so that the division truncates and 0 shares are minted to the victim front-ran depositor, and the attacker can back-run this deposit and burn all of her shares to receive both her assets and the victim’s assets.

Such an attack is not likely to occur with the other paths, as _deposit implements a 0.01 ether minimum deposit requirement which guarantees the fraction.base to be at least 0.01 ether if it’s non-zero. This in turn makes the truncation requirement:

assets * 0.01 ether (minimum) < fraction.elastic, and as assets  can be at least 0.01 ether through the other paths than redeemCream, this requires fraction.elastic to be larger than 0.01 ether ^2 in the best case for the attacker, which is 10^14 ether which is larger than the current mainnet Ether supply, rendering this attack practically impossible for the other functions.

redeemCream function also does not implement the sandwich protection mechanism of lastDeposit even though this mechanism itself is vulnerable as explained in https://github.com/kebabsec/review-manifold/issues/32, after its patched it should be applied to redeemCream as well.

Recommendation: Patch redeemCream so that it also implements a minimum deposit requirement like _deposit, also implement sandwich protection like _deposit.

19. [LOW] maxWithdraw Is not being used internally

#31

Context: MevEth.sol#L558-L561

Severity: Low

Description: maxWithdraw is not used internally and therefore should be external instead of public.

Recommendation: Change public to external.

20. [MEDIUM] Sandwich protection in _withdraw can be bypassed

#32

Context: MevEth.sol#L584-L586

Severity: Medium

Description: The _withdraw function contains a protection mechanism against sandwich attacks as well as prevent flash mints to end up withdrawn. The current implementation assumes that msg.sender same block. This can be easily bypassed by using a separate contract or by manipulating the receiver and owner parameters. the following Proof Of Concept demonstrates few ways .

  • Transfering: Depositor transfers received shares, making receiver to withdraw.,
  • Receiveing: _deposit allows specifying the receiver who may not be the msg.sender and receiver could initiate the withdrawal on the same block.
  • Approving: A depositor can grants allowance of received shares which simply withdraws on behalf of owner.

Recommendation:

Extend Validation Checks. receiver and owner needs to be validated against deposits done on the last block. receiver from deposits cannot withdraw, and neither on behalf of owner.

In _withdraw:

1
2
3
4
5

// Extend sandwich protection to cover receiver and owner
blockNumber = block.number;
if ( (_isZero(blockNumber - lastDeposit[msg.sender]) || _isZero(blockNumber - lastDeposit[owner])) && _isZero(blockNumber - lastRewards)) {
revert MevEthErrors.SandwichProtection();
}

In _deposit:

1
2
3

// Record last deposit block for receiver and msg.sender
lastDeposit[receiver] = block.number;
lastDeposit[msg.sender] = block.number;

Note that transfer and transferFrom are not accounting for block, and requires additional changes. Revisit the roles of receiver and owner in deposit and withdraw to ensure they cannot be misused. Using a Time/Block-based mechanism prevent immediate withdrawals.

21. [HIGH]MevEthShareVault functions payValidatorWithdraw and logRewards are order dependent

#34

Context: MevEthShareVault.sol#L120,

Description:

MevEthShareVault functions payValidatorWithdraw and logRewards depend on correct calling order to prevent accounting corruption. Calling logRewards when there is withdrawn validator stake in the contract will result in the stake amount to be registered as protocolBalance.rewards. This can be an issue if there is a full beacon chain withdrawal right as the operator calls logRewards. Consider the following scenario:

  1. There is 8 ETH in the contract.
  2. Operator calls logRewards(2 eth) with the intention to register 2 eth as protocol fee and 6 eth as rewards.
  3. Right before the operator tx is processed, a full beacon chain withdrawal of 32 eth occurs.
  4. This results in 2 eth being registered as protocol fee, 38 eth as rewards, and 0 eth as withdrawal amount.

Recommendation:

TBD. The accounting of logRewards and payValidatorWithdraw should be combined in a single function. This has other caveats. We will update this recommendation.

22. [INFORMATIONAL] Typos

#35

MevEth.sol#L639-L642 Description: In line 624, “Mevth” should be “MevEth”.

MevEth.sol#L62-L63, MevEth.sol#L66-L67 Description: In line 62 and line 66, there is a typo, “comitted” should be “committed”.

MevEthShareVault.sol#L115-L116) Description: Line 116 contains a typo, “Cahce” should be “cache”

MevEthShareVault.sol#L89 Description: This comment has a typo “prorotocol” should be “protocol”

23. [INFORMATIONAL] Unused imports

#36

Context: MevEth.sol#L25-L28 Severity: Informational Description: There’s unused imports in this contract. Recommendation: Remove unused imports.

Context: WagyuStaker.sol#L8-L11 Severity: Informational Description: Unused import. Recommendation: Remove unused imports

24. [LOW] Unstable dependency versions used

#37

Context: lib/

Description:

Contract dependencies should always use audited release code. But versions of Solmate and OpenZeppelin included in lib/ are unstable, non-release code. When importing core dependencies, developers should deliberately choose which release version to import, instead of simply importing latest version from the main/master branch.

Recommendation:

Install dependencies using release tags (e.g.: forge install transmissions11/solmate@v7), and do not update dependencies without deliberation.

25. [MEDIUM]payValidatorWithdraw might cause temporary partial DOS for MevEthShareVault due to insolvency

#38

Context: MevEthShareVault#L156

Description: payValidatorWithdraw has a balance sanity check to make sure that payValidatorWithdraw has enough ETH balance so that it can send 32 ethers to mevEth.

1

        if (exitSize > address(this).balance) revert MevEthErrors.NotEnoughEth();

Yet MevEthShareVault has the additional liabilities of protocolBalance.rewards and protocolBalance.fees which are increment-only and the only way to decrement them is by paying them out fully. As this is the case, if there are protocolBalance.rewards and protocolBalance.fees which are accumulated but not paid out (meaning that the values are non-zero), and payValidatorWithdraw occurs, the contract may wrongly send the rewards and fees as a withdraw action of 32 ethers, which would lead to those rewards not being accounted as fraction.elastic in mevEth and as they are not from an actual withdrawal they are not reflected in fraction.base either, so the users cannot receive this yield. We think the only way to “salvage” these ethers is by spinning up a new validator with them through createValidator.

(Note that this recovery action can be performed by calling createValidator to an existing validator and then waiting for a partial withdrawal through createValidator but this is assumed to be an issue itself in this audit, thus the phrasing above.)

It should be noted that if such a recovery mechanism is to be used, the users would receive less rewards in the short term, but as the value is not lost and simply auto-compounded without consent, we consider that this warrants only a medium severity issue as value is not lost.

Recommendation: Change the line to

1

        if (exitSize > address(this).balance + protocolBalance.rewards + protocolBalance.fees) revert MevEthErrors.NotEnoughEth();

26. [HIGH] Calling createValidator on an existing validator may lead to loss of user funds

#39

Context: MevEth.sol#L309

Description: “Beacon Deposit” contract of Ethereum staking does not check if a call of deposit is made to an existing (active) validator or a fresh (inactive) one. Due to this it is possible for createValidator to be called multiple times for the same validator assuming that there are enough ethers. As Beacon Chain regularly sweeps excess ethers in validators (if a validator’s balance is more than 32 ether it is considered a reward and a partial withdrawal action is initiated), the excess 32 ether would be considered as a reward and would be accounted in fraction.elastic even though they are also accounted in fraction.base. This would inflate the corresponding ether amounts of shares, and a malicious existing user of the system can utilize this to steal the ethers of other people.

PoC Scenario:

  1. 64 ether are accumulated in mevEth. fraction.base == fraction.elastic == 64 ether at this point. Let’s assume there are two holders for simplicity, with 1 share each.
  2. Operators want to spin up two new validators, but by operational errors, createValidator is called on the same validator twice. The validator’s balance is 64 ether, the sweeping occurs and 32 ether is sent back to the protocol as rewards. fraction.elastic == 96 ether at this point.
  3. Assume the validator exits. The actual balance is 64 ether, and normally 32 ether belongs to first holder while the the other 32 belongs to the other holder.
  4. Malicious first holder burns his share to get half of fraction.elastic which is 48 ether, even though they only supplied 32 initially. The extra 16 ether is stolen from the second holder.

Recommendation: Keep a mapping of deposited validator public keys so that createValidator is not called on the same validator twice.

27. [MEDIUM] Hardcoding the withdrawal size of a validator can lead to problems

#40

Context: MevEth.sol#L350

Description: Both MevEth and MevEthShareVault hardcodes the withdrawal size of a validator as 32 ether, yet a validator may exit with less due to penalties or slashing. This hardcoding may lead the funds to be stuck indefinitely in their respective contract and users not being able to withdraw. Alternatively, the maximum effective balance of a validator might be increased due to potential Ethereum updates, which would make withdrawals larger than 32 ether possible, which would require changes to the hardcoding of 32 ether.

Recommendation: Account for other possible withdrawal sizes.

Manifold: Acknowledged.

This design was chosen to simplify accounting and limit risks. To expand, here is the previous version and the issues we had with it:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15

        // If the msg.value is 32 ether, the elastic should not be updated.
        if (msg.value == 32 ether) {
            return;
        }

        // If the msg.value is less than 32 ether, the elastic should be reduced.
        if (msg.value < 32 ether) {
            /// @dev Elastic will always be at least equal to base. Base will always be at least equal to the MIN_DEPOSIT amount.
            // assume slashed value so reduce elastic balance accordingly
            fraction.elastic -= uint128(32 ether - msg.value);
        } else {
            // If the msg.value is greater than 32 ether, the elastic should be increased.
            // account for any unclaimed rewards
            fraction.elastic += uint128(msg.value - 32 ether);
        }
  • simplify accounting 1 validator created = 32 eth, 1 validator exited = 32 eth
  • operator mistaken amount calls would cause irreversible price changes
  • any call effecting fraction.elastic, effects the price and is vulnerable to front-running
  • as long as net rewards outweighs net slashing, reward payouts can account for slashing by subtracting slashed amounts on exit from next reward payouts.
    • Note this is an assumption that currently holds true with a large margin of error

28. [LOW]createValidator should assert that the activated validator withdraws to cleared addresses

#41

Context: MevEth.sol#L309

Description: As can be deduced from grantValidatorWithdraw of MevEth:

1
2

        // Check that the sender is the staking module or the MevEthShareVault.
        if (!(msg.sender == address(stakingModule) || msg.sender == mevEthShareVault)) revert MevEthErrors.InvalidSender();

It is accepted that validators can only withdraw to stakingModule or mevEthShareVault address, including the case mevEthShareVault actually being a multisig.

Yet, createValidator does no assertions on if newData.withdrawal_credentials is one of these two addresses or not, meaning that it is possible by operator error to configure a validator that withdraws to another address. This would mean that the user deposited ether that is also reflected in fraction.base cannot be retrieved back easily and may lead to temporary denial of service. If such a scenario occurs, and assuming the operators are not malicious and it was caused by an error, the validator that is wrongly configured may exit and if the wrong withdrawal address is also controlled by the team they can send these otherwise lost ether to the staking module and payValidatorWithdraw can be called so that the accounting is not affected. On the other hand, if the withdrawal address were to be set to an address that is not controlled by the team the funds may get lost.

Recommendation: Assert newData.withdrawal_credentials is either staking module or the share vault.