hats-finance / Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990

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

Clear Pending Governance After Completed Transfer of Governance #19

Open hats-bug-reporter[bot] opened 8 months ago

hats-bug-reporter[bot] commented 8 months ago

Github username: @0xmahdirostami Submission hash (on-chain): 0x964ec7737b8648c2e93847dce5038fb0208bc8e117ebcceb4326486c68449cd6 Severity: low

Description: Description\ The contract uses a two-step process for transferring governance, but after the acceptGovernance function is called, it doesn't delete pending governance.

Impact\ While this vulnerability may not have a significant impact, it is recommended to clear states and follow the rules in the OpenZeppelin Ownable2Step.

Attachments\

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

    function acceptGovernance() external {
        require(
            pendingGovernance == msg.sender,
            "DappnodeSmoothingPool::acceptGovernance: Only pending governance"
        );
    
        governance = pendingGovernance;
    +       delete pendingGovernance;
        emit AcceptGovernance(pendingGovernance);
    }
0xmahdirostami commented 8 months ago

Revised Code File (Optional)

I didn't consider the event in my report, this is a correct recommended code:

    function acceptGovernance() external {
        require(
            pendingGovernance == msg.sender,
            "DappnodeSmoothingPool::acceptGovernance: Only pending governance"
        );

        governance = pendingGovernance;
        emit AcceptGovernance(pendingGovernance);
+       delete pendingGovernance;
    }
invocamanman commented 8 months ago

I think this issue should be categorized as an informational or recommendation more than a bug. I implemented the other way, since it's more optimal (if it's gonna be transferred more than once). And there's no security issue since the pending governance will match the current governance. The only "damage" that could be caused is that the governance acts "maliciously" and "accepts" the governance multiple times, but this does not damage the system in any way.