manifoldfinance / mevETH2

mevETH LST Protocol - Repo has migrated see link
https://github.com/MEV-Protocol/meveth
27 stars 2 forks source link

Audit: MANETH-9 #139

Closed sandybradley closed 1 year ago

sandybradley commented 1 year ago

DISCREPANCIES BETWEEN DOCUMENTATION AND IMPLEMENTATION

SEVERITY: Informational

DESCRIPTION

  1. The Design Doc mentions “there is the ManifoldOwner role”, however this role is not implemented in the code. Instead we have the admin role in Auth.sol
  2. Implementation of fallback is not according to docs. In the Token Design section of the README.md it says “…mev-eth also supports deposits via its fallback (recieve technically) function for call-data free deposits.” This is not implemented in the code, as fallback() and receive() don’t contain any logic to support deposit into the ERC4626 mechanism and the is actually limited to accepting transactions only from the WETH contract.
  3. Documentation inconsistent with implementation for MevEthShareVault beneficiary. MevEthShareVault line 58-59 states fund recovery will be sent to admin, however beneficiary is a different address than admin. If not carefully added, the beneficiary address could lack the ability to transfer ETH, consequently making the funds locked.
  4. Incorrect documentation in WagyuStaker constructor. depositContract and mevEth are incorrectly documented in lines 39-40. Mistaking such variables for the wrong values could lead to absolute malfunction.
  5. logRewards() documentation doesn't agree with implementation. The @notice for logRewards() states “the RewardPayment event is emitted” L111 However, this is not true, as the event emitted is RewardsCollected L137.
    
    // Keeps track of all admins
    mapping(address => bool) public admins;
    modifier onlyAdmin() {
    if (!admins[msg.sender]) {
    revert Unauthorized();
    }
    _;
    }
    /// @dev Only Weth withdraw is defined for the behaviour. Deposits should be directed to deposit / mint.
    Rewards via grantRewards and validator withdraws
    /// via grantValidatorWithdraw.
    receive() external payable {
    if (msg.sender != address(WETH)) revert MevEthErrors.InvalidSender();
    }
    /// @dev Only Weth withdraw is defined for the behaviour. Deposits should be directed to deposit / mint.
    Rewards via grantRewards and validator withdraws
    /// via grantValidatorWithdraw.
    fallback() external payable {
    if (msg.sender != address(WETH)) revert MevEthErrors.InvalidSender();
    }

// Catch the error and send to the admin for further fund recovery bool success = payable(beneficiary).send(_rewards); /// @param depositContract The address of the WETH contract to use for deposits /// @param mevEth The address of the WETH contract to use for deposits /// @notice Function to log rewards, updating the protocol balance. Once all balances are updated, the RewardPayment event is emitted. emit RewardsCollected(protocolFeesOwed, rewardsEarned - protocolFeesOwed);

1. Rename the admin role or reference it accordingly in the docs.
2. Refactor docs to reflect actual implementation.
3. Change nomenclature to reflect actual variables being used.
4. Change to constructor documentation to:
```solidity
/// @param depositContract The Canonical Address of the BeaconChainDepositContract
/// @param mevEth The address of the MevEth contract
  1. Correct documentation or emit appropriate event.
sandybradley commented 1 year ago

Updated docs needed.