Audit: MANETH-41

[0xKitsune]

Status

Reported

Type

Vulnerability

Severity

Highest

Code Snippet:

function grantRewards() external payable {
    // Process the withdrawal queue, paying out any pending withdrawal tickets before updating the fraction.
    processWithdrawalQueue();
    if (!(msg.sender == address(stakingModule) || msg.sender == mevEthShareVault)) revert MevEthErrors.InvalidSender();

    /// @dev Note that while a small possiblity, it is possible for the MevEthShareVault rewards + fraction.elastic to overflow a uint128.
    /// @dev in this case, the grantRewards call will fail and the funds will be secured to the MevEthShareVault.beneficiary address.
    fraction.elastic += uint128(msg.value);
    emit Rewards(msg.sender, msg.value);
}

Remediation

The root of the issue is how rewards distribution is currently designed and also a common challenge with liquid staking protocols.

The share rate should be based on the existing funds as well as any unrealised profit from validator nodes. This is similar to how lending protocols should take any outstanding debt and accumulated interest into account even if the loan has not yet been repaid.

The unrealised profit could be calculated from an APR based on all previous rewards or on a constant rate multiplied by the number of validators (and assuming they do not get penalised on the consensus layer).

Description

MevEth.sol:grantRewards (L323-332)

The staking protocol obtains rewards from MEV and node operators validating blocks on the consensus layer. Rewards are accumulated in the MevEthShareVault contract. Then, the operator of the contract calls MevEthShareVault.sol:payRewards to submit a batch of rewards to MevEth via grantRewards.

This process batches rewards together in one single update and therefore does not linearly divide rewards among stakers. The MevEth contracts also allows for instant deposits and withdrawals (flash-staking).

It becomes possible for a user to steal the rewards (depending on their capital) without taking on any risk:

1. Front-run or sandwich the call to payRewards
2 .Call MevEth:deposit with a very large amount of ETH.
3. After execution of payrewards
4. Call MevEth:withdraw to re-obtain the initial stake plus a large chunk of the rewards depending on the initial stake amount.

In other words, the user briefly becomes the user with the largest stake and will receive a portion of the rewards equal to that stake percentage. They can then immediately unstake again.

The user is risk-free from any risk that a normal taker would be exposed to, such as validator slashing and protocol exploits.

[sandybradley]

Note that this would not be an issue if the operator uses a mev-relay as standard.

Another mitigation technique is not allow a user to deposit and withdraw in the same block.

Personally I prefer using exact accounting as we do instead of smoothing with unrealized profits / losses, as this too would have attack vectors and generally favour shorter term holders over longer term.

[sandybradley]

Example of denying a user of depositing and withdrawing in the same block:

mapping(address => uint256) lastTransactionBlock;

function deposit(uint256 amount) public {
    require(lastTransactionBlock[msg.sender] != block.number, "You can't deposit and withdraw in the same block");

    // Your deposit logic here

    // Update last transaction block for the user
    lastTransactionBlock[msg.sender] = block.number;
}

function withdraw(uint256 amount) public {
    require(lastTransactionBlock[msg.sender] != block.number, "You can't deposit and withdraw in the same block");

    // Your withdrawal logic here

    // Update last transaction block for the user
    lastTransactionBlock[msg.sender] = block.number;
}

[sandybradley]

Another mitigation technique (slightly better than above) is to deny withdrawals within a certain time period.

uint64 public LOCKUP = 1 days;
mapping(address => uint256) public depositTimestamps;

function _updateDepositTimestamp(address account, uint256 shares) internal {
    // Set the deposit timestamp for the user
    uint256 prevBalance = balanceOf[account];
    if (_isZero(prevBalance)) {
        depositTimestamps[account] = block.timestamp;
    } else {
        // multiple deposits, so weight timestamp by amounts
        unchecked {
            depositTimestamps[account] = ((depositTimestamps[account] * prevBalance) + (block.timestamp * shares)) / (prevBalance + shares);
        }
    }
}

function deposit(uint256 amount) public {   
    // Your deposit logic here

    _updateDepositTimestamp(msg.sender, amount);
}

function withdraw(uint256 amount) public {
    if (block.timestamp < depositTimestamps[msg.sender] + LOCKUP) revert InsufficientLockupTime();

    // Your withdrawal logic here
}

In both cases, transfer and transferFrom would need to be overriden to update these mappings.

[sandybradley]

A better mitigation than the 2 above is to implement a withdraw fee

[sandybradley]

@aldoborrero @ControlCplusControlV @0xKitsune Thoughts?

[ControlCplusControlV]

I'm not sure, adding a fee on Withdraw seems like it could cause a lot of issues and potentially lead to a lot of weird ERC4626 bugs

[sandybradley]

Withdraw fees are normal for ERC4626. The specs mention specifically where withdraw fees should be included. https://eips.ethereum.org/EIPS/eip-4626

[sandybradley]

Implementing the fee on withdraw does not effect passing all external ERC4626 tests: https://github.com/manifoldfinance/mevETH2/pull/164