GoGoPool contest - 0xmint's results

Lines of code

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

Vulnerability details

In createMinipool, an event is emitted with details of a newly created minipool.

This includes relevant information that a subsequent user can utilise to create another minipool.The only condition that prevents a minipool from being created again with the same nodeID on a second subsequent call to createMinipool is that the status would remain the same(invalid, as there's no change to the state ) so therefore would revert. The only valid state transition in a PreLaunch is a ‘Launch’ or if the minipool is being ‘Cancelled’.

The only way for the minipool to move back to PreLaunch is after either of these transitions.

From the aforementioned change in the minipool status, it is important to focus on recordStakingEnd() as this ensures that staking for the Node Operator is complete and records the rewards (if validation period is successful ) for the node operator. It also sets the status to ‘Withdrawable’. This will allow the owner of the minipool to be able to withdraw their avax node operator rewards(if any).

The recordStakingEnd() also emits an event similar to createMinipool() so, a malicious user could wait until after this function is called and then create a minipool with the same exact nodeID as there is nothing preventing another user from reusing a previous nodeID.

This would be problematic as this would

arbitrarily reset the data previously stored for a node operator who first used that nodeID and replace it with information pertaining to another user.

This would prevent the node operator from withdrawing their previously deposited avax plus any avax rewards(will remain in the staking and vault contracts ) since they are no longer the owner of the index that the nodeID is linked to . Additionally, the avax rewards earned that was previously reset will never get compounded for the node operator in recreateMiniPool. It would've been recorded and be attributable to the node operator(first user of the nodeID) if they were able to successfully call withdrawMinipoolFunds().

See the following adjustment to testWithdrawMinipoolFunds()in 2022-12-gogopool/test/unit/MinipoolManager.t.sol that demonstrates this issue.

The following solves the issue :

if (nodeIDOwner[nodeID]==address(0)) { nodeIDOwner[nodeID ]=msg.sender ; } else { address nodeOwner=nodeIDOwner[nodeID]; if(msg.sender!=nodeOwner) revert invalidNodeOwner() ; }

Add the above to createMinipool() and create the necessary mapping as per the logic.