recordStakingError() missing execution decreaseMinipoolCount()

[code423n4]

Lines of code

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

Vulnerability details

Impact

recordStakingError() no reduction minipoolcount resulting in no stake, and you can still get rewards

Proof of Concept

minipoolCount will be increased or decreased during state transfer
1.->Prelaunch: minipoolCount++
2.->Withdrawable: minipoolcount--
3.->Canceled: minipoolCount--
Other states do not operate on minipoolCount

However, according to the existing state transfer, the following path exists:

->Prelaunch(+1)->Launched->ERROR->Finished

If the above path is executed, minipoolCount will be increased by 1, because ERROR does not reduce minipoolcount.

minipoolCount represents how many minipools the user currently has, and when it equals 0, it will no longer be able to get rewards.
The RewardsStartTime is set to 0 when minipoolCount==0 by the following control

    function calculateAndDistributeRewards(address stakerAddr, uint256 totalEligibleGGPStaked) external onlyMultisig {
...
        uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
        if (minipoolCount == 0) {
            staking.setRewardsStartTime(stakerAddr, 0); //****@audit minipoolCount==0， set RewardsStartTime =0
        }
    }

    function isEligible(address stakerAddr) external view returns (bool) {
...
        //***@audit rewardsStartTime==0 ,no Eligible
        return (rewardsStartTime != 0 && elapsedSecs >= dao.getRewardsEligibilityMinSeconds());
    }            

This will result in the minipoolCount always being greater than 0, although there is no more stake, you can still get the reward.

test code:

forge test --match testMinipoolCountError

diff --git a/MinipoolManager.t.sol.orig b/MinipoolManager.t.sol
index ed2a6e8..a055d9c 100644
--- a/MinipoolManager.t.sol.orig
+++ b/MinipoolManager.t.sol
@@ -577,6 +577,43 @@ contract MinipoolManagerTest is BaseTest {
 		assertEq(mp1finished.status, uint256(MinipoolStatus.Finished));
 	}
 
+	function testMinipoolCountError() public {
+		uint256 duration = 2 weeks;
+		uint256 depositAmt = 1000 ether;
+		uint256 avaxAssignmentRequest = 1000 ether;
+		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
+		uint128 ggpStakeAmt = 200 ether;
+
+		vm.startPrank(nodeOp);
+		ggp.approve(address(staking), MAX_AMT);
+		staking.stakeGGP(ggpStakeAmt);
+		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
+		vm.stopPrank();
+
+		assertEq(staking.getMinipoolCount(nodeOp), 1); //****@audit ->PreLPrelaunch , MinipoolCount++
+
+
+		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
+		vm.prank(liqStaker1);
+		ggAVAX.depositAVAX{value: MAX_AMT}();
+
+		vm.prank(address(rialto));
+		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
+
+		bytes32 txID = keccak256("txid");
+		vm.prank(address(rialto));
+		minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
+
+		bytes32 errorCode = "INVALID_NODEID";
+
+		vm.prank(address(rialto));
+		minipoolMgr.recordStakingError{value: validationAmt}(mp1.nodeID, errorCode);
+
+		vm.prank(nodeOp);
+		minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
+		assertEq(staking.getMinipoolCount(nodeOp), 1); //****@audit when Finished , still MinipoolCount==1
+	}
+

 	function testBondZeroGGP() public {
 		vm.startPrank(nodeOp);
 		address nodeID = randAddress();

Tools Used

Recommended Mitigation Steps

    function recordStakingError(address nodeID, bytes32 errorCode) external payable {
...
        Staking staking = Staking(getContractAddress("Staking"));
        staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
+       staking.decreaseMinipoolCount(owner);

    

[emersoncloud]

duplicate of #235

[c4-judge]

GalloDaSballo marked the issue as duplicate of #235

[c4-judge]

GalloDaSballo marked the issue as satisfactory