GoGoPool contest - sk8erboy's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L196 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L242 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L259 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L51

Vulnerability details

Impact

The method createMinipool allows for relaunching an existing pool, but it does not check that the relaunch is being triggered by the pool's owner. Instead, it just resets the pool, increases the pool count for the msg.sender, and on line 259 sets the new owner to be the msg.sender. There needs to be some owner validation for the case of an existing pool, because this should not be possible, and actually results in bad accounting (keep reading).

The pool needs to be in a state that allows transitioning into the Prelaunched state, otherwise this call will fail. This includes the Error state. However, the transition of a pool to Error state also does not decrement number of user minipools (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484). The method will remove the avax, but it will not decrement the number of pools. From the Error state, the pool can be re-launched by a different owner. Both the original and new owner will have non-zero minipool count in the staking, and the original owner will have this count forever, because they will not have a pool to cancel any more. This can be problematic as the original owner seemingly always have some pools, and their staking time would never reset to 0 in the calculateAndDistributeRewards() methods in ClaimNodeOp.sol#82 would be always true. This would also result in the staker ALWAYS being eligible for rewards as ClaimNodeOp.isEligible() would always have non-zero value. This can result in wrong distribution of rewards.

Proof of Concept

Sequence of actions described above. 1. Alice creates a minipool. 2. Rialto changes the pool's state to Error. 3. Mallory re-creates Alice's minipool. Alice now does not own the pool, but Mallory does, and both have number of minipools set to 1 in Staking.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Add validation of an owner to when the existing pool is re-created. Furthermore, ensure integrity in the book keeping of the number of pools, because it can currently be violated (even if the owner is validated).

Lines of code

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

Vulnerability details

Impact

Method requireNextActiveMultisig() always returns the first active multisig, not the next active one. If there is any intention of cycling multisigs (as the naming or engineering of the code suggests), this is a bug. The same multisig would be by default used for all pools.

Proof of Concept

None needed

Tools Used

None needed

Recommended Mitigation Steps

Re move the bug. Keep a counter of the latest multisig used, and loop from there mod count of the multisigs.

Lines of code

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

Vulnerability details

Impact

The transition of a pool to Error state also does not decrement number of user minipools (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484). As a result, a the book keeping of the minipool numbers can be wrong, becuase users can relaunch pools by calling the createMinipool() method which will blindly increment the number of their pools. Also, all the other methods for finishing pools decrement the number of the staker in the staking contract, but this one does not.

Proof of Concept

None needed.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Decrese the number of user minipools in staking when bringing the pool into the errored state.