GoGoPool contest - kaliberpoziomka8552's results

Lines of code

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

Vulnerability details

Impact

By calling the function createMinipool(...) the minipool with status Withdrawable may be overwritten to a new minipool with the same minipoolIndex which will cause losing funds that should be withdrawn.

Proof of Concept

The function createMinipool(...) may create a new minipool with the minipoolIndex the same as existing minipool (here). However, the function requireValidStateTransition(...) that checks the valid state transition, allows for changing the state to be changed from Withdrawable to PreLaunch (here), which is the state that is set by createMinipool(...). This effectively allows for overwriting existing pool in Withdrawable state and makes it impossible to withdraw funds from the minipool by the owner.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider not allowing the minipool in Withdrawable state to be overwritten with the createMinipool(...) function.

Lines of code

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

Vulnerability details

Impact

Any address may overwrite any existing minipool in the state Finished, Canceled, Error or Withdrawable. A malicious user may overwrite the minipool in Withdrawable state, causing a loss of funds for the current owner of minipool.

Proof of Concept

The function createMinipool(...) allows for updating existing minipool (here). However it is not checked if the caller of createMinipool(...) is the owner of this minipool. That allows any address to overwrite an existing minipool, if this minipool is in state: Finished, Canceled, Error or Withdrawable, effectively resetting all minipool's data and setting the caller as an owner. Since minipool in state Withdrawable may be overwtiten to a new minipool (described in issue: "Minipool with state Withdrawable can be overwritten") this leads to possibility of blocking node operator from withdrawing funds, consider scenario:

• node operator creates minipool for nodeID = 1
• minipool enters the state Withdrawable
• malicious user calls createMinipool(...) with nodeID = 1
• node operator loses funds provided to the minipool (min. 1000 ETH) and rewards

Note: malicious user must be staking ggp token.

Tools Used

Manual review

Recommended Mitigation Steps

When updating the minipool with createMinipool(...), consider asserting that the caller is the owner of updated minipool.

Lines of code

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

Vulnerability details

Impact

Using the function finishFailedMinipoolByMultisig(...) when minipool is in error state, will block the node operator from withdrawing the deposit.

Proof of Concept

The function finishFailedMinipoolByMultisig(...) changes the state of minipool from Error to Finished. When minipool is in the state Finished the node operator can't withdraw deposit, since withdrawing deposit is possible only by calling cancelMinipool(...) or withdrawMinipoolFunds(...) i.e. from states: Prelaunch, Withdrawable or Error.

Tools Used

Manual review

Recommended Mitigation Steps

Consider returning deposit amount to the owner of minipool in function finishFailedMinipoolByMultisig(...).

Lines of code

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

Vulnerability details

Impact

Most minipools will have the same multisig, which makes one address responsible for most minipools management.

Proof of Concept

During creation of minipool, the multisig address is returned from multisigManager.requireNextActiveMultisig(). However, the function requireNextActiveMultisig() always returns first enabled multisig, which will result in most minipools having the same multisig address.

Tools Used

Manual review

Recommended Mitigation Steps

Consider diversifying multisig addresses assigned to minipools.

Lines of code

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

Vulnerability details

Impact

The node operator may withdraw the same AVAX deposit amount multiple times.

Proof of Concept

Recreated minipool still has the originally deposited AVAX amount in storage, even though this deposit was already withdrawn by calling withdrawMinipoolFunds(...) (here). This allows for multiple withdrawals of deposit amount of AVAX but only once sending AVAX to protocol (on creation of minipool). Consider scenario:

• node operator creates minipool and deposits 1000 AVAX
• minipool enters Withdrawable state and the node operator withdraws deposit and rewards
• minipool multisig address calls recreateMinipool(...) to recreate minipool
• after some time, when minipool enters Withdrawable state again, the node operator can withdraw deposit again

The amount of provided deposit is only reset when the minipool is updated with createMinipool(...), but not with recreateMinipool(...).

Tools Used

Maunal review

Recommended Mitigation Steps

Consider resetting the amount of the deposit when recreating the minipool.

Lines of code

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

Vulnerability details

Impact

Setting minipool into Error state and withdrawing funds, would cause incorrect minipool count.

Proof of Concept

After the function recordStakingError(...) is called by the multisig, the node operator may withdraw funds only by calling withdrawMinipoolFunds(...). This way the minipool count for the node operator is not decreased, because it is only decreased by calling cancelMinipool(...) and recordStakingEnd(...).

Tools Used

Maunal review

Recommended Mitigation Steps

Consider decreasing minipool count in the function recordStakingError(...) or allowing node operator to call cancelMinipool(...) after the error is registered.