Disabled multisig can still perform crictical pool management actions such as initiate staking, record staking start, record staking end etc

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L68

Vulnerability details

Impact

I presume disabling a multisig feature in Line 68 of MultisigManager is a security feature to prevent compromised/inactive multisigs.

However, a disabled multisig can currently perform mission critical operations such as minipool initiation, canceling multipools, allocating rewards etc. This is a significant security risk, both to the node operator and liquid staker. Since exploiting this can only be an inside job, I'm classifying it as MEDIUM risk (medium only because of the mission criticality of operations controlled by a pool multisig)

Proof of Concept

Guardian appoints Bob as multisig - Bob is tasked to maintain 5 minipools. Bob eventually leaves the core team and his account becomes inactive & hence disabled. Bob's multisig, although disabled, can still record start/end of staking, recreate minipools for the 5 minipools he controls. There is no provision currently to transfer multisigs of individual pools to a currently active multisig.

If Bob turns a malicious actor, all minipools assigned to Bob and the associated liquid staked AVAX and node operator staked AVAX/ GGP rewards are at risk.

Tools Used

Recommended Mitigation Steps

Dev team has selectively implemented access controls - while a disabled multisig cannot set GGP-AVAX price or distribute GGP rewards, he can still continue to manage minipools. This selective approach on multisig access is inconsistent and risky - at the very minimum, I recommend to define a function with onlyGuardian access in MinipoolManager that gets called every time a multisig is disabled.

This function should loop over all pools & replace the disabled multisig with a currently active one. Since the number of minipools is easily known by getMinipoolCount & getMinipools will give us a list of all pools, such a function should be trivial to implement.

On a risk/reward spectrum, investing efforts in building a function that completely removes access of disabled multisigs makes absolute sense.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #618

[c4-judge]

GalloDaSballo marked the issue as duplicate of #702

[GalloDaSballo]

See #618

[c4-judge]

GalloDaSballo marked the issue as partial-50