Minipool whose multisig has been disabled cannot be reassigned a valid one

[code423n4]

Lines of code

https://github.com/multisig-labs/gogopool/blob/3b5ab1d6505ef9be6197c4056acd38d6bed4aff6/contracts/contract/MinipoolManager.sol#L273

Vulnerability details

Impact

The likelihood of startRewardsCycle() reverting due to division by zero is practically not going to happen. This is because disableAllMultisigs() is only reasonably invoked when pauseEverything() is called by the defender. At his point, startRewardsCycle() will have already been paused. Logically, enableMultisig() will be called to enable the registered multisigs prior to calling resumeEverything(). Otherwise, it would not make much sense with the core functionalities in MinipoolManager still crippled considering many of the methods there, i.e. canClaimAndInitiateStaking(), claimAndInitiateStaking(), recordStakingStart(), recordStakingEnd(), recordStakingEndThenMaybeCycle(), recreateMinipool(), recordStakingError(), and cancelMinipoolByMultisig() are onlyValidMultisig() dependent.

But, if the Guardian decides not to enable certain registered multisig, getting the minipools associated manually reassigned to a new and valid multisig is going to be a problem.

Proof of Concept

No where in the code bases is found a method that could have multisigAddr in the Minipool struct reassigned a valid multisig address. The only place where a multisig can be assigned to a minipool is located in the function logic of createMinipool():

File: MinipoolManager.sol#L273

		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig);

Creating a new minipool via createMinipool() with a new nodeID will not do the trick either since onlyValidMultisig() is minipoolIndex specific.

Devoid of a valid multisig, the affected minipools will be denied of completing validations on top of accessing other associated functions with onlyValidMultisig() visibility.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider implementing a restricted setter function to have the multisig of affected minipools reset as follows:

    function setMultisigAddr(address nodeID) external onlyGuardian {
        uint256 minipoolIndex = getIndexOf(nodeID);
        MultisigManager multisigManager = MultisigManager(getContractAddress("MultisigManager"));
	address multisig = multisigManager.requireNextActiveMultisig();
        setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig);

[0xju1ie]

As far as I can tell, this is working as designed. In our system, a disabled multisig means a multisig that won't be assigned new minipools, not one that can't complete validation for it's current minipools. A disabled multisig is still a valid multisig.

Let me know if there is something I am misunderstanding from this issue. For now I am disputing

[c4-sponsor]

0xju1ie marked the issue as sponsor disputed

[raymondfam]

I now see it after looking more carefully and deeper into the codes. I was simply misguided by the function NatSpec comment:

https://github.com/multisig-labs/gogopool/blob/4bcef8b1d4e595c9ba41a091b2ebf1b45858f022/contracts/contract/MultisigManager.sol#L67

	/// @dev this will prevent the multisig from completing validations. The minipool will need to be manually reassigned to a new multisig

The comment should probably be revised to prevent any confusion here.

That said, how will the disabled multisig be incentivized to continue taking care of the assigned minipools if this disabled address no longer gets compensated with GGP via distributeMultisigAllotment()?

Shouldn't the affected minipools' multisigAddr be reassigned to the next enabled multisig via requireNextActiveMultisig()?

[GalloDaSballo]

This would be closer to Original-M-11

Which we must consider a known issue which the sponsor chose to nofix

I will double check but I think I must close as OOS

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope

[GalloDaSballo]

Closing per the above as OOS