GoGoPool contest - wallstreetvilkas's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

1.Any validator, while creating a pool can submit different validator's nodeId and change the pool's status to prelaunch. It can only change to prelaunch if the current pool's status is withdrawable. Now imagine if the attacker submits nodeId of the validator's pool which is withdrawable. He cannot withdraw, because while withdrawing, it reverts because it's not a valid pool state transition. 2. The pool's data gets reset, clearing out the previous pool data set by victim validator leading to a loss of validator funds.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

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

The if statement checks if a pool for NodeId exists. It incorrectly assumes that the person calling the function uses their own NodeId. It then resets the pool associated with NodeId, which isn't the caller, but some innocent validator. All the attacker needs to do is to just call the function createMinipool() and set the NodeId to someone else's. That's it.

Tools Used

Manual review

Recommended Mitigation Steps

Check if there's already an existing pool associated with specified NodeId, and then check if msg.sender is the actual owner of the pool or not.