code423n4 commented 1 year ago

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).