Introduction & Scope
This audit looks into the following contracts of mevETH2:
Note: This report does not provide any guarantee or warranty of security for the project.
Table of Contents
- [INFORMATIONAL] Mismatch with access control
i++Can be unchecked for gas savings
- [INFORMATIONAL] Removed feature still referred in the comments
- [LOW] Missing address check in constructor of
send()Has hardcoded amount of gas it allows to use
- [INFORMATIONAL] Unnecessary variable logging in
- [LOW] Events should be emitted for change in variables
- [INFORMATIONAL] Redundant subtraction in
- [INFORMATIONAL] Redundant casting when assigning
- [LOW] Missing zero address check for
- [LOW] Withdraw event does not account for fees
redeemCreamAllows for minting of
mevETHeven when contract is paused
_isZeroNot more efficient than equality check
- [LOW] Using type
uint256for all constants is more efficient
allowanceShould not be decreased if a user has max allowance
redeemCreamfunction is not working as
creamTokencannot be transferred to
redeemCreamis vulnerable to a sandwich inflation attack
maxWithdrawIs not being used internally
- [MEDIUM] Sandwich protection in
_withdrawcan be bypassed
logRewardsare order dependent
- [INFORMATIONAL] Typos
- [INFORMATIONAL] Unused imports
- [LOW] Unstable dependency versions used
payValidatorWithdrawmight cause temporary partial DOS for
MevEthShareVaultdue to insolvency
- [HIGH] Calling
createValidatoron an existing validator may lead to loss of user funds
- [MEDIUM] Hardcoding the withdrawal size of a validator can lead to problems
createValidatorshould assert that the activated validator withdraws to cleared addresses
1. [INFORMATIONAL] Mismatch with access control
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.
onlyOperator modifier for an
onlyAdmin modifier, or equivalent to such.
i++ Can be unchecked for gas savings
Description: In this situation,
++i can be unchecked as overflow is practically impossible.
Recommendation: Possibly uncheck to save gas
Description: The location of
data would be better if it was
calldata rather than
memory, as it would save gas.
Recommendation: Possibly change
4. [INFORMATIONAL] Removed feature still referred in the comments
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
Description: The input variable
_mevEth is not checked for zero address.
Recommendation: Add a zero address check to avoid errors.
send()Has hardcoded amount of gas it allows to use
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
.call() should be used instead. Alternatively, Solmate’s
safeTransferETH can also be used.
7. [INFORMATIONAL] Unnecessary variable logging in
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
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
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 in the event instead of
rewardsEarned - protocolFeesOwed.
10. [INFORMATIONAL] Redundant casting when assigning
Description: In these lines, the castings are redundant, as
pendingStakingModule is already declared as of type
Recommendation: just using
stakingModule = pendingStakingModule; should have the intended effect.
11. [LOW] Missing zero address check for
Description: It is perhaps better to have a zero address check for the variable
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
12. [LOW] Withdraw event does not account for fees
Description: Line 613 emits
Withdraw, but this does not take fees into account.
convertToShares(assets) != shares in this context as
redeem passes down fee adjusted values for these variables.
Recommendation: Account for the fees in this event.
redeemCream Allows for minting of
mevETH even when contract is paused
Description: While both
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
_stakingUnpaused() so it also pauses minting, and use it in this function.
_isZero Not more efficient than equality check
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
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.
allowance Should not be decreased if a user has max allowance
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.
redeemCream function is not working as
creamToken cannot be transferred to
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
Recommendation: Instead, you could add
and then use
in the function to achieve the same effect of transfer to
redeemCream is vulnerable to a sandwich inflation attack
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
_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:
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.elastic is non-zero and
assets * fraction.base is smaller than
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.base to 1 wei. Then the attacker can inflate
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.
redeemCream so that it also implements a minimum deposit requirement like
_deposit, also implement sandwich protection like
maxWithdraw Is not being used internally
maxWithdraw is not used internally and therefore should be
external instead of
20. [MEDIUM] Sandwich protection in
_withdraw can be bypassed
_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
owner parameters. the following Proof Of Concept demonstrates few ways .
- Transfering: Depositor transfers received shares, making receiver to withdraw.,
_depositallows specifying the
receiverwho may not be the
receivercould initiate the withdrawal on the same block.
- Approving: A depositor can grants allowance of received shares which simply withdraws on behalf of
Extend Validation Checks.
owner needs to be validated against deposits done on the last block. receiver from
deposits cannot withdraw, and neither on behalf of owner.
transferFrom are not accounting for block, and requires additional changes. Revisit the roles of
owner in deposit and withdraw to ensure they cannot be misused. Using a Time/Block-based mechanism prevent immediate withdrawals.
logRewards are order dependent
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:
- There is 8 ETH in the contract.
- Operator calls
logRewards(2 eth)with the intention to register
2 ethas protocol fee and
6 ethas rewards.
- Right before the operator tx is processed, a full beacon chain withdrawal of 32 eth occurs.
- This results in
2 ethbeing registered as protocol fee,
38 ethas rewards, and
0 ethas withdrawal amount.
TBD. The accounting of
payValidatorWithdraw should be combined in a single function. This has other caveats. We will update this recommendation.
22. [INFORMATIONAL] Typos
MevEth.sol#L639-L642 Description: In line 624, “Mevth” should be “MevEth”.
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
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
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.
Install dependencies using release tags (e.g.:
forge install transmissions11/solmate@v7), and do not update dependencies without deliberation.
payValidatorWithdraw might cause temporary partial DOS for
MevEthShareVault due to insolvency
payValidatorWithdraw has a balance sanity check to make sure that
payValidatorWithdraw has enough ETH balance so that it can send 32 ethers to
MevEthShareVault has the additional liabilities of
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.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
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
(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
26. [HIGH] Calling
createValidator on an existing validator may lead to loss of user funds
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.
- 64 ether are accumulated in
fraction.base == fraction.elastic == 64 etherat this point. Let’s assume there are two holders for simplicity, with 1 share each.
- Operators want to spin up two new validators, but by operational errors,
createValidatoris 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 etherat this point.
- 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.
- Malicious first holder burns his share to get half of
fraction.elasticwhich 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
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.
This design was chosen to simplify accounting and limit risks. To expand, here is the previous version and the issues we had with it:
- 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
createValidator should assert that the activated validator withdraws to cleared addresses
Description: As can be deduced from
It is accepted that validators can only withdraw to
mevEthShareVault address, including the case
mevEthShareVault actually being a multisig.
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.
newData.withdrawal_credentials is either staking module or the share vault.