Audit: MANETH-21

[sandybradley]

DISCREPANCY IN AUTH BETWEEN CONTRACTS COULD CAUSE MALFUNCTION

SEVERITY: Low

PATH: see description

REMEDIATION

implement a single Auth contract that uses OpenZeppelin’s Access Control with roles. The roles should clearly defined privileges regarding the functions in each of the protocol contract. MevEth, MevEthShareVault and WagyuStaker should refer to this contract for access control checks

DESCRIPTION

Auth values for admins and operators is not shared between MevEth, MevEthShareVault and WagyuStaker. There is an admin and operator for each contract, this is same name but they have different privileges. If the protocol grants admin in each contract to the same account, then that would be inefficient and might lead to errors if the protocol wants to remove/change the admin (it would require 3 calls to deleteAdmin and/or 3 calls to addAdmin). Malicious and/or removed admins in MevEth for example could still have access to MevEthShareVault functions as their removal would not be atomic. The same is valid for operators. Which if removed from WagyuStaker could still createValidator() in MevEth. Currently there is no automated way to ensure these privileged accounts are always in sync

contract WagyuStaker is Auth, IStakingModule {

contract MevEthShareVault is Auth, ReentrancyGuard, IMevEthShareVault {

abstract contract Fee is Auth {

[sandybradley]

I don't see a need to use OpenZeppelin's roles. The trade-off seems to be:

• either deploy Auth.sol as an external library linked to the 3 contracts
  • solves operator / admin update consistency
  • reduce contract deployment sizes
  • makes every call using Auth more expensive in gas because of external calls
• or leave Auth.sol internally imported, as is, in 3 separate contracts
  • would need to make 3 calls for each operator / admin update
  • Auth related calls are cheaper in gas because they are internal

[0xKitsune]

Deploying Auth.sol as an external lib would definitely be expensive, however if admin functions are only being called occasionally, this might be an acceptable trade off.

One potential option is to deploy an AuthManager contract that would orchestrate all Admin changes throughout all of the contracts atomically. The AuthManager would just have to be added as an admin on each of the contracts once.

[sandybradley]

I like the idea of an auth manager. Auth setup can remain the same, with the AuthManager registered as an admin. e.g.

contract AuthManager {
    address public auth;
    address public mevEth;
    address public mevEthShareVault;
    address public wagyuStaker;

    error Unauthorized();

    constructor(address initialAdmin, address initialMevEth, address initialShareVault, address initialStaker) {
        auth = initialAdmin;
        mevEth = initialMevEth;
        mevEthShareVault = initialShareVault;
        wagyuStaker = initialStaker;
    }

    modifier onlyAuth() {
        if (msg.sender != auth) {
            revert Unauthorized();
        }
        _;
    }

    function updateMevEth(address newMevEth) external onlyAuth {
        mevEth = newMevEth;
    }

    function updateMevEthShareVault(address newMevEthShareVault) external onlyAuth {
        mevEthShareVault = newMevEthShareVault;
    }

    function updateWagyuStaker (address newWagyuStaker ) external onlyAuth {
        wagyuStaker = newWagyuStaker ;
    }

     /*//////////////////////////////////////////////////////////////
                           Maintenance Functions
    //////////////////////////////////////////////////////////////*/
    function addAdmin(address newAdmin) external onlyAuth {
        IAuth(mevEth).addAdmin(newAdmin);
        IAuth(mevEthShareVault).addAdmin(newAdmin);
        IAuth(wagyuStaker).addAdmin(newAdmin); 
    }

    function deleteAdmin(address oldAdmin) external onlyAuth {
        IAuth(mevEth).deleteAdmin(oldAdmin);
        IAuth(mevEthShareVault).deleteAdmin(oldAdmin);
        IAuth(wagyuStaker).deleteAdmin(oldAdmin); 
    }

    function addOperator(address newOperator) external onlyAuth {
        IAuth(mevEth).addOperator(newOperator);
        IAuth(mevEthShareVault).addOperator(newOperator);
        IAuth(wagyuStaker).addOperator(newOperator); 
    }

    function deleteOperator(address oldOperator) external onlyAuth {
        IAuth(mevEth).deleteOperator(oldOperator);
        IAuth(mevEthShareVault).deleteOperator(oldOperator);
        IAuth(wagyuStaker).deleteOperator(oldOperator); 
    }
}

[ControlCplusControlV]

I like the AuthManager approach a lot, then we just collectively call to this from each different contract, although we don't need our manager contracts there do we (eg WagyuStaker + MevShareVault upgrades)

[sandybradley]

It's more simple than that even. Let me have a go.

[sandybradley]

https://github.com/manifoldfinance/mevETH2/pull/168