Audit: MANETH-34

[0xKitsune]

Status

Reported

Type

Vulnerability

Severity

High

Code Snippet:

function payRewards() external onlyOperator {
        // Cache the rewards balance.
        uint256 _rewards = address(this).balance - balance;

        // Send the rewards to the MevEth contract
        try ITinyMevEth(MEV_ETH).grantRewards{ value: _rewards }() { }
        catch {
            // Catch the error and send to the admin for further fund recovery
            bool success = payable(beneficiary).send(_rewards);
            if (!success) revert MevEthErrors.SendError();
        }

        // Emit an event to track the rewards paid
        emit RewardsPaid(_rewards);
    }

Remediation

Description

Path: WagyuStaker.sol

Operator and beneficiary might collude to steal the reward.

When an external call is made inside ITinyMevEth(MEV_ETH).grantRewards() 63/64 of gas amount is used for an external call according to EIP-150, leaving 1/64 to the caller.

Therefore the attacker can wisely choose gas amount for the inception WagyuStaker.sol:payRewards() }}call such that an external call {{ITinyMevEth(MEV_ETH).grantRewards() reverts due to out of gas. But still there is enough gas to send ETH inside catch .

It could be the case because MevEth.sol:grantRewards() calls processWithdrawalQueue() inside to process the withdrawal queue which might be gas-consuming.

[sandybradley]

beneficiary is initially set as the admin and can only be updated by the admin, so this issue can be mitigated by making sure the operator and admin are properly separated.

[sandybradley]

This beneficiary issue was created because of 2 possibilities of locked eth.

• mevEth is upgraded and it's address changes
  • this could be mitigated by having the mevEth address upgradeable in MevEthShareVault

• if MevEthShareVault is upgraded it will not be authorised any longer to call grantRewards

  /// @notice Grants rewards from the MevEthShareVault, updating the fraction.elastic.
  /// @dev Before updating the fraction, the withdraw queue is processed, which pays out any pending withdrawals.
  /// @dev This function is only callable by the MevEthShareVault.
  function grantRewards() external payable {
      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);
  }

Is the sender check necessary for grantRewards? Perhaps not. Does it matter who grants rewards? If there is a zero msg.value check I can't see any attack vector.

In the case above, beneficiary can be safely removed.

[sandybradley]

@0xKitsune