After claiming ownership the pending should be deleted

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x98b362c2886afc2e38829c82e96919cb7b3c8b6c110ebd0cd35cc505b58aebfc Severity: medium

Description: Description\

This is actually a low severity issue submitted with the intent the protocol team should fix in contracts.

OwnableMaster.proposeOwner() is used by Master to propose the new Master of overall contract. proposedMaster address is set by calling this function.

The issue here is after the proposedMaster claimed the ownership, the address remains with proposedMaster which should be deleted after claiming ownership.

    function claimOwnership()
        external
        onlyProposed
    {
        master = proposedMaster;        

@audit // does not delete proposedMaster

        emit ClaimedOwnership(
            master,
            msg.sender
        );
    }

Recommendation

It is recommended to delete the proposedMaster address after ownership claim.

    function claimOwnership()
        external
        onlyProposed
    {
        master = proposedMaster;        
+      delete proposedMaster;

        emit ClaimedOwnership(
            master,
            msg.sender
        );
    }

[0xRizwan]

Here, it is correctly done. For reference

[vm06007]

@0xRizwan why do you think it is wrong? Since proposeMaster will be same as current master after ownership is claimed there is no downside of keeping propose as previous candidate. Because this can only be overwritten once new master is propose. Hence I find this implementation is correct, and the one you've provided is not optimal one because it tend to waste more gas for no good reason. The one implemented needs less gas because it does not need to delete previous proposedMaster as it serves no purpose. Hence our initial implementation is more optimized and the one you are proposing is less optimize. You can provide example where this is an issue (if you can)

So I'll ask you based on your comment: The issue here is after the proposedMaster claimed the ownership, the address remains with proposedMaster which should be deleted after claiming ownership.

why do you think it is an "issue" and why it "should be deleted"