hats-finance / Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990

Dappnode's MEV Smoothing Pool
0 stars 2 forks source link

Consider implementing two-step procedure for updating protocol addresses #42

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @saidqayoumsadat Submission hash (on-chain): 0xd1fb4bc8c511cebae1c60963c1bf2ad21c027f5b9a0fdd1463c1b05fdf2ea4d2 Severity: low

Description: Description

A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the 'set' functions ensure that the recipient is of the right interface type.

file: /contracts/DappnodeSmoothingPool.sol

182    function initialize(
        address _governance,
        uint256 _subscriptionCollateral,
        uint256 _poolFee,
        address _poolFeeRecipient,
        uint64 _checkpointSlotSize,
        uint64 _quorum
    ) external initializer {
        // Initialize requires
        require(
            _poolFee <= 10000,
            "DappnodeSmoothingPool::initialize: Pool fee cannot be greater than 100%"
        );

        require(
            _quorum != 0,
            "DappnodeSmoothingPool::initialize: Quorum cannot be 0"
        );

        // Set initialize parameters
        governance = _governance;
        subscriptionCollateral = _subscriptionCollateral;

        checkpointSlotSize = _checkpointSlotSize;
        quorum = _quorum;

        poolFee = _poolFee;
        poolFeeRecipient = _poolFeeRecipient;
        deploymentBlockNumber = block.number;

https://github.com/hats-finance/Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990/blob/3929e24ea288d697d38948b8690c8c2028e5042b/contracts/DappnodeSmoothingPool.sol#L182-L210

file:

544    function transferGovernance(
        address newPendingGovernance
    ) external onlyGovernance {
        pendingGovernance = newPendingGovernance;
        emit TransferGovernance(newPendingGovernance);

https://github.com/hats-finance/Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990/blob/3929e24ea288d697d38948b8690c8c2028e5042b/contracts/DappnodeSmoothingPool.sol#L544

invocamanman commented 1 year ago

The governance might or might not implement the EIP-165. Also this transaction will be submitted through a multisig so will be double checked by the members of the multisig, plus there are some tooling in the multisig to actually simulate a transaction/set of transactions. For this, i think our security mesures are good enough, and more flexible than the ones proposed