GoGoPool contest - Faith's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L390 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L127-L135

Vulnerability details

Details

Currently, when a minipool is created by a node operator, a Rialto multisig owner is assigned as the multisig for this minipool. This multisig owner is the only one that can:

1. Start the staking phase of the minipool
2. End the staking phase
3. Record an error if there was one while attempting to stake
4. Recreate minipools
5. Some other miscellaneous things

Whenever one of these functions are called, an onlyValidMultisig() function is used to ensure that the msg.sender is the multisig owner that was assigned to this minipool. The implementation of this function is here.

When a multisig owner address is added to a minipool (inside the createMinipool() function here), the function checks to ensure that this multisig owner address is "enabled". This implies that multisig owners may be disabled (presumably if an owner decides they don't want to be a part of the project anymore).

Now, it is safe to assume that if a multisig owner is "disabled", then they shouldn't be interacting with the minipool at all in the first place. This is especially a problem if they intend on being malicious.

One way to be malicious here would be, for example, to not do anything at all. Since there isn't any functionality to reassign the multisig owner that's assigned to a minipool, this would mean that any minipools that have been assigned to them (which means AVAX has been staked for it from the node operator and liquid staker) would be remain assigned forever. Unless the other owners can convince the assigned (now ex) owner to end the staking on the pool, this would end up with funds being stuck forever in all the assigned minipools.

Impact

A malicious owner whose multisig access has been disabled is still able to perform operations on minipools assigned to them. This allows them to call recordStakingError() at any time to get staked rewards lost forever.

Furthermore, assuming they may be a disgruntled ex-owner (i.e something didn't go their way, so they were "kicked" from the Rialto multisig), they don't need to do anything at all. If that happens, the minipool would just stay assigned forever, and the 1000 (or more) AVAX will be lost forever, both for the node operator and the liquid staker, as the only way to get it back is for the assigned multisig owner to call recordStakingEnd() (or any of the other functions).

I'm setting this as high severity due to the situation of funds being stuck forever.

Proof of concept

I modified test/unit/MinipoolManager.t.sol to showcase that disabling the multisig does not do anything, as the test still passes successfully.

$ git diff test/unit/MinipoolManager.t.sol
diff --git a/test/unit/MinipoolManager.t.sol b/test/unit/MinipoolManager.t.sol
index ed2a6e8..725dfd3 100644
--- a/test/unit/MinipoolManager.t.sol
+++ b/test/unit/MinipoolManager.t.sol
@@ -415,6 +415,10 @@ contract MinipoolManagerTest is BaseTest {
                minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, 9 ether);

                //right now rewards are split equally between the node op and user. User provided half the total funds in this test
+               vm.stopPrank();
+               vm.prank(guardian);
+               multisigMgr.disableMultisig(address(rialto));
+               vm.startPrank(address(rialto));
                minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
                uint256 commissionFee = (halfRewards * 15) / 100;
                //checking the node operators rewards are corrrect

While doing this, I also noted this comment above the disableMultisig() function that states that the assigned multisig will need to be reassigned. Since there is no functionality for this, I think it is safe to assume that the minipool will stay stuck in whatever state its in.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing functionality to allow assigned multisigs on minipools to be reassigned by any multisig owner.