GoGoPool contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L379-L383

Vulnerability details

Impact

ProtocolDAO implementation does not have a method to take out GGP. So it can't handle ggp unless it updates ProtocolDAO

Proof of Concept

recordStakingEnd() will pass the rewards of this reward "If the validator is failing at their duties, their GGP will be slashed and used to compensate the loss to our Liquid Stakers"

At this point slashGGP() will be executed and the GGP will be transferred to "ProtocolDAO"

staking.slashGGP():

    function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
        Vault vault = Vault(getContractAddress("Vault"));
        decreaseGGPStake(stakerAddr, ggpAmt);
        vault.transferToken("ProtocolDAO", ggp, ggpAmt);
    }

But the current ProtocolDAO implementation does not have a method to take out GGP. So it can't handle ggp unless it updates ProtocolDAO

Tools Used

Recommended Mitigation Steps

1.transfer GGP to ClaimProtocolDAO or 2.Similar to ClaimProtocolDAO, add spend method to retrieve GGP

contract ProtocolDAO is Base {
...

+    function spend(
+        address recipientAddress,
+        uint256 amount
+    ) external onlyGuardian {
+        Vault vault = Vault(getContractAddress("Vault"));
+        TokenGGP ggpToken = TokenGGP(getContractAddress("TokenGGP"));
+
+        if (amount == 0 || amount > vault.balanceOfToken("ProtocolDAO", ggpToken)) {
+            revert InvalidAmount();
+        }
+
+        vault.withdrawToken(recipientAddress, ggpToken, amount);
+
+        emit GGPTokensSentByDAOProtocol(address(this), recipientAddress, amount);
+   }

Judge has assessed an item in Issue #510 as 3 risk. The relevant finding follows:

In red are the state transitions that can only be performed with special privileges

recreateMinipool(): The following transitions will be performed Withdrawable->PreLaunch Error->PreLaunch

createMinipool(): will perform the following transition: Finished->PreLaunch Canceled->PreLaunch

There is a problem with both methods here. The methods only verify that the state can be converted, not that the current state is legal For example:

If the current state is Finished, you can still call recreateMinipool() If the current state is Withdrawable, you can still call createMinipool() This will result in: 1:recreateMinipool() can be front-run by executing recordStakingEnd() to get back the stake first, and then executing recreateMinipool() will get more stake 2:the new stake will overwrite the old stake, the old stake has not been retrieved, resulting in the loss of

Lines of code

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

Vulnerability details

Impact

recordStakingEnd() may not be executed when duration > 365 days

Proof of Concept

recordStakingEnd() is called at the end of the cycle and passes Reward, which is used to reward staking

"If the validator is failing at their duties, their GGP will be slashed and used to compensate the loss to our Liquid Stakers"

is executed by staking.slashGGP(owner, slashGGPAmt);

At that time, slashGGPAmt may be larger than the owner's GGPStake, resulting in an overflow: Because.

The formula for calculating the number of slashGGPAmt is:

ExpectedAVAXRewardsRate * duration / 365 days

ExpectedAVAXRewardsRate is generally set at 10%, the same as MinCollateralizationRatios

So that if duration > 365 days, it will result in slashGGPAmt > GGPStake number staking.slashGGP() will revert:Arithmetic over/underflow causing recordStakingEnd() to fail all the time

test code $ forge test --match testRecordStakingEndWithSlashWithUnderflow

	function testRecordStakingEndWithSlashWithUnderflow() public {
		uint256 duration = 366 days;  //****@audit  more then 365 day***/
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether; //**** 10% ****//

		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		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);

		//will fail
		vm.expectRevert(MinipoolManager.InvalidMultisigAddress.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

		//will fail
		int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
		store.setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Error));
		vm.prank(address(rialto));
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
		store.setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Staking));

		vm.startPrank(address(rialto));
		vm.expectRevert(MinipoolManager.InvalidEndTime.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

		skip(duration);

		vm.expectRevert(MinipoolManager.InvalidAmount.selector);
		minipoolMgr.recordStakingEnd{value: 0 ether}(mp1.nodeID, block.timestamp, 0 ether);

		vm.expectRevert(MinipoolManager.InvalidAmount.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 9 ether);

		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

		MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
		assertEq(mp1Updated.status, uint256(MinipoolStatus.Withdrawable));
		assertEq(mp1Updated.avaxTotalRewardAmt, 0);
		assertTrue(mp1Updated.endTime != 0);

		assertEq(mp1Updated.avaxNodeOpRewardAmt, 0);
		assertEq(mp1Updated.avaxLiquidStakerRewardAmt, 0);

		assertEq(minipoolMgr.getTotalAVAXLiquidStakerAmt(), 0);

		assertEq(staking.getAVAXAssigned(mp1Updated.owner), 0);
		assertEq(staking.getMinipoolCount(mp1Updated.owner), 0);

		assertGt(mp1Updated.ggpSlashAmt, 0);
		assertLt(staking.getGGPStake(mp1Updated.owner), ggpStakeAmt);
	}

Tools Used

Recommended Mitigation Steps

if slashGGPAmt> GGPStake , use GGPStake

    function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
        Vault vault = Vault(getContractAddress("Vault"));
+       if (getGGPStake(stakerAddr)<ggpAmt){
+          ggpAmt = getGGPStake(stakerAddr);
+       }
        decreaseGGPStake(stakerAddr, ggpAmt);
        vault.transferToken("ProtocolDAO", ggp, ggpAmt);
    }

Judge has assessed an item in Issue #615 as 2 risk. The relevant finding follows:

3:upgradeExistingContract() need unregisterContract() first and then registerContract(). Avoid newAddr==existingAddr. unregisterContract() remove newAddr

function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {

•   unregisterContract(existingAddr);
    registerContract(newAddr, newName);
  

•   unregisterContract(existingAddr);
  

  }

Judge has assessed an item in Issue #510 as 2 risk. The relevant finding follows:

If the current state is Withdrawable, you can still call createMinipool() This will result in: 1:recreateMinipool() can be front-run by executing recordStakingEnd() to get back the stake first, and then executing recreateMinipool() will get more stake 2:the new stake will overwrite the old stake, the old stake has not been retrieved, resulting in the loss of

test code for: Prelaunch->Launched->Staking->Error->Finished(front-run)-> execute recreateMinipool()->Prelaunch

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 testFrontRunRecreateMinipool() public {

•   uint256 duration = 4 weeks;
  

•   uint256 depositAmt = 1000 ether;
  

•   uint256 avaxAssignmentRequest = 1000 ether;
  

•   uint256 validationAmt = depositAmt + avaxAssignmentRequest;
  

•   // Enough to start but not to re-stake, we will add more later
  

•   uint128 ggpStakeAmt = 100 ether;
  

•   vm.startPrank(nodeOp);
  

•   ggp.approve(address(staking), MAX_AMT);
  

•   staking.stakeGGP(ggpStakeAmt);
  

•   MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
  

•   vm.stopPrank();
  

•   address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
  

•   vm.prank(liqStaker1);
  

•   ggAVAX.depositAVAX{value: MAX_AMT}();
  

•   vm.prank(address(rialto));
  

•   minipoolMgr.claimAndInitiateStaking(mp.nodeID);
  

•   bytes32 txID = keccak256("txid");
  

•   vm.prank(address(rialto));
  

•   minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
  

•   skip(duration / 2);
  

•   // Give rialto the rewards it needs
  

•   uint256 rewards = 10 ether;
  

•   deal(address(rialto), address(rialto).balance + rewards);
  

•   // Pay out the rewards
  

•   vm.prank(address(rialto));
  

•   minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);
  

•   // Add a bit more collateral to cover the compounding rewards
  

•   vm.prank(nodeOp);
  

•   staking.stakeGGP(1 ether);
  

•   //***audit:frontrun finshed
  

•   vm.prank(nodeOp);
  

•   minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
  

•   MinipoolManager.Minipool memory oldMp = minipoolMgr.getMinipoolByNodeID(mp.nodeID);
  

•   assertEq(staking.getAVAXStake(oldMp.owner), 0);  //***audit:AVAXStake == 0
  

•   vm.prank(address(rialto));
  

•   minipoolMgr.recreateMinipool(mp.nodeID);
  

•   assertEq(staking.getAVAXStake(mp.owner), oldMp.avaxNodeOpRewardAmt); //***audit:AVAXStake >0
  

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

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L41-L43

Vulnerability details

Impact

When more than 10 mulitsig, it is impossible to modify or delete the old ones, making it impossible to create new valid ones.

Proof of Concept

MultisigManager limits the number of Multisig to 10, which cannot be deleted or replaced after they have been disable This will have a problem, if the subsequent use of 10, all 10 for some reason, be disabled Then it is impossible to add new ones and replace the old ones, so you have to continue using the old Multisig at risk

    function registerMultisig(address addr) external onlyGuardian {
        int256 multisigIndex = getIndexOf(addr);
        if (multisigIndex != -1) {
            revert MultisigAlreadyRegistered();
        }
        uint256 index = getUint(keccak256("multisig.count"));
        if (index >= MULTISIG_LIMIT) {
            revert MultisigLimitReached(); //***@audit limt 10, and no other way to delete or replace the old Multisig ***//
        }

Tools Used

Recommended Mitigation Steps

add replace old mulitsig method

    function replaceMultisig(address addr,address oldAddr) external onlyGuardian {
        int256 multisigIndex = getIndexOf(oldAddr);
        if (multisigIndex == -1) {
            revert MultisigNotFound();
        }

        setAddress(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".address")), addr);
        emit RegisteredMultisig(addr, msg.sender);
    }

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);