GoGoPool contest - immeas's results

Lines of code

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

Vulnerability details

Description

A node operator sends in the amount of duration they want to stake for. Behind the scenes Rialto will stake in 14 day cycles and then distribute rewards.

If a node operator doesn't have high enough availability and doesn't get any rewards, the protocol will slash their staked GGP. For calculating the expected rewards that are missed however, the full duration is used:

File: MinipoolManager.sol

557:	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
558:		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
559:		uint256 rate = dao.getExpectedAVAXRewardsRate();
560:		return (avaxAmt.mulWadDown(rate) * duration) / 365 days; // full duration used when calculating expected reward
561:	}

...

670:	function slash(int256 index) private {

...

673:		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
674:		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
675:		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt); // full duration
676:		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

This is unfair to the node operator because the expected rewards is from a 14 day cycle.

Also, If they were to be unavailable again, in a later cycle, they would get slashed for the full duration once again.

Impact

A node operator staking for a long time is getting slashed for an unfairly large amount if they aren't available during a 14 day period.

The protocol also wants node operators to stake in longer periods: https://multisiglabs.notion.site/Known-Issues-42e2f733daf24893a93ad31100f4cd98

Team Comment:

• This can only be taken advantage of when signing up for 2-4 week validation periods. Our protocol is incentivizing nodes to sign up for 3-12 month validation periods. If the team notices this mechanic being abused, Rialto may update its GGP reward calculation to disincentive this behavior.

This slashing amount calculation incentives the node operator to sign up for the shortest period possible and restake themselves to minimize possible losses.

Proof of Concept

Test in MinipoolManager.t.sol:

	function testRecordStakingEndWithSlashHighDuration() public {
		uint256 duration = 365 days;
		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();

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

		skip(2 weeks); // a two week cycle
		
		vm.prank(address(rialto));
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

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

		int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
		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);

		// log slash amount
		console.log("slashedAmount",mp1Updated.ggpSlashAmt);
	}

Slashed amount for a 365 days duration is 100 eth (10%). However, where they to stake for the minimum time, 14 days the slashed amount would be only ~3.8 eth.

Tools Used

vs code, forge

Recommended Mitigation Steps

Either hard code the duration to 14 days for calculating expected rewards or calculate the actual duration using startTime and endTime.

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#L262-L263

Vulnerability details

Description

A user can try to recreate their pool using createMinipool supplying enough funds to start the pool again. If they do this without calling withdrawMinipoolFunds first their previously staked funds will be locked in the vault.

State transitions to Prelaunch from Withdrawable and Error are allowed:

File: MinipoolManager.sol

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

Since createMinipool overwrites the previous staked amount with the new, what was staked previously is lost since both withdrawMinipoolFunds and _cancelMinipoolAndReturnFunds looks at avaxNodeOpAmt to determine how much funds to transfer back to the node operator:

File: MinipoolManager.sol

262:		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);
263:		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), avaxAssignmentRequest);

Impact

If a node operator calls createMinipool on a pool that is in state Withdrawable or Error their previously staked funds will be locked in the vault.

I don't know if this will be handled in UI or not, hence submitting this as medium instead of QA.

Proof of Concept

PoC test in MinipoolManager.t.sol

	function testError_createMinipoolByNodeOp() public {
		// same setup as testFullCycle_Error
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);
		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;
		uint256 originalRialtoBalance = address(rialto).balance;

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

		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		// Assume something goes wrong and we are unable to launch a minipool
		bytes32 errorCode = "INVALID_NODEID";

		minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);
		assertEq(address(rialto).balance - originalRialtoBalance, 0);
		
		// NodeOps funds should be back in vault
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
		
		mp = minipoolMgr.getMinipool(mp.index);

		// pool in state error
		assertEq(mp.status, uint256(MinipoolStatus.Error));
		vm.stopPrank();

		vm.startPrank(nodeOp);
		mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		

		// funds in the vault
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt * 2);

		skip(5 days);

		// cancel
		minipoolMgr.cancelMinipool(mp.nodeID);

		// originally staked funds still in vault
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
	}

The same is true if the pool actually goes through a staking cycle.

Tools Used

vs code, forge

Recommended Mitigation Steps

Don't allow state transitions from Withdrawable or Error to Prelaunch

or

If the protocol wants to allow this functionality, have createMinipool add to avaxNodeOpAmt and avaxLiquidStakerAmt instead of overwriting them.

Lines of code

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

Vulnerability details

Description

When a pool is set to error the funds are returned back to the vault:

File: MinipoolManager.sol

484:	function recordStakingError(address nodeID, bytes32 errorCode) external payable {
485:		int256 minipoolIndex = onlyValidMultisig(nodeID);
486:		requireValidStateTransition(minipoolIndex, MinipoolStatus.Error);

...

502:        // Send the nodeOps AVAX to vault so they can claim later
503:		Vault vault = Vault(getContractAddress("Vault"));
504:		vault.depositAVAX{value: avaxNodeOpAmt}(); // funds returned to vault
505:
506:		// Return Liq stakers funds
507:		ggAVAX.depositFromStaking{value: avaxLiquidStakerAmt}(avaxLiquidStakerAmt, 0);
508:
509:		Staking staking = Staking(getContractAddress("Staking"));
510:		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
511:
512:		subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);
513:
514:		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Error);
515:	}

From here the node operator can call withdrawMinipoolFunds to have their staked funds (and any reward) transferred to them. However, Rialto can also finish the minipool by calling finishFailedMinipoolByMultisig.

But finishFailedMinipoolByMultisig will not transfer any funds to the node operator. But, since it has changed the state to Finished it is no longer possible for the node operator to retrieve their funds.

finishFailedMinipoolByMultisig relies on that the minipool is in the same states as withdrawMinipoolFunds so any successfully call to it will lock the node operators funds in the vault.

Impact

Calling finishFailedMinipoolByMultisig will lock the node operators staked funds in the vault.

Proof of Concept

PoC test in MinipoolManager.t.sol

	function testError_FinishByMultisig() public {
		// same setup as testFullCycle_Error
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);
		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;
		uint256 originalRialtoBalance = address(rialto).balance;

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

		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		// Assume something goes wrong and we are unable to launch a minipool
		bytes32 errorCode = "INVALID_NODEID";

		minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);
		assertEq(address(rialto).balance - originalRialtoBalance, 0);
		
		// NodeOps funds should be back in vault
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
		
		mp = minipoolMgr.getMinipool(mp.index);

		// pool in state error
		assertEq(mp.status, uint256(MinipoolStatus.Error));
		
		// Rialto calls to finish the pool
		minipoolMgr.finishFailedMinipoolByMultisig(mp.nodeID);
		vm.stopPrank();

		// state Finished
		mp = minipoolMgr.getMinipool(mp.index);
		assertEq(mp.status, uint256(MinipoolStatus.Finished));

		// funds still in vault
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

		// node operator cannot withdraw their funds
		vm.prank(nodeOp);
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
	}

Tools Used

vs code, forge

Recommended Mitigation Steps

Have finishFailedMinipoolByMultisig return the funds, and possible reward, from the vault to the owner of the minipool.

Lines of code

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

Vulnerability details

Description

Rialo can recreate a pool where the node operator no longer has any stake:

When recreate is called, it checks if the state transition to Prelaunch is valid:

File: MinipoolManager.sol:

163:		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
164:			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
165:		} else if (currentStatus == MinipoolStatus.Finished || currentStatus == MinipoolStatus.Canceled) {
166:			// Once a node is finished/canceled, if they re-validate they go back to beginning state
167:			isValid = (to == MinipoolStatus.Prelaunch);
168:		} else {
169:			isValid = false;
170:		}

Finished is reached when a node operator has withdrawn their stake. Hence has no more AVAX in the vault. But it is allowed to transition to Prelaunch from both Finished and Withdrawable (and Canceled and Error).

A node operator can withdraw their stake before the recreate call, then when Rialo recreates the pool AVAX staked from other node operators will be used. As long as the owner of the pool have a good enough GGP collateral. Which, if this is their only "stake", only requires a non-zero amount of GGP staked.

Impact

When Rialo recreates a pool the node operator of that pool can stake using AVAX from other node operators waiting to stake.

And when staking is done, withdraw their AVAX.

Calling this high because of this comment in discord:

JohnnyGault — 12/30/2022 3:22 PM

To clarify duration for everyone -- a nodeOp can choose a duration they want, from 14 days to 365 days. But behind the scenes, Rialto will only create a validator for 14 days. At the end of the 14 days, the Avalanche protocol pays out the validation rewards and all funds are returned to the contracts, which divvy up the rewards between liq stakers and nodeops. If the duration still has time left to go, Rialto will call recreateMinipool which will do another 14 days cycle( if it can considering GGP collat and available liq staker funds), compounding the rewards. In this way, the protocol recvs yield from every minipool every 14 days, and the collat ratio of GGP can never get more than 14 days out of whack. In the future we see a path for laddering maturities to maximize yield and still provide necessary liq for stakers.

This means that you can do this every 14 days and only provide liquidity once.

Proof of Concept

PoC test in MinipoolManager.t.sol:

	function testRecreateMinipoolAndStartAfterWithdraw() public {
		// same setup as testRecreateMinipool
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 101 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);

		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);

		vm.prank(address(rialto));
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);

		// nodeOp withdraws
		vm.prank(nodeOp);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);

		// rialto recreates the pool
		vm.prank(address(rialto));
		minipoolMgr.recreateMinipool(mp.nodeID);

		vm.prank(address(rialto));
		// claimAndInitiateStaking will fail because there is no funds in the vault
		vm.expectRevert(Vault.InsufficientContractBalance.selector);
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		address otherNodeOp = address(0x1337);
		vm.deal(otherNodeOp,MAX_AMT);
		vm.prank(guardian);
		ggp.transfer(otherNodeOp, MAX_AMT);

		vm.startPrank(otherNodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt + 1 ether);
		// another node op starts a pool
		createMinipool(depositAmt + 5.75 ether, avaxAssignmentRequest + 5.75 ether, duration);
		vm.stopPrank();

		vm.prank(address(rialto));
		// claim and stake will take the other nodeOps staked avax, and staked ggp
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
	}

Tools Used

vs code, forge

Recommended Mitigation Steps

use a new state for recreated pools, Recreated and only allow transition to it from Error and Withdrawable

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

L-2 no way to remove compromised/broken multisigs without upgrading the contract

Lines of code

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

Vulnerability details

Description

When creating a minipool the node operator is required to put up a collateral in GGP, the protocol token. The amount of GGP collateral needed is currently calculated to be 10% of the AVAX staked. This is calculated using the price of GGP - AVAX.

If the node operator doesn't have high enough availability and doesn't get any rewards the protocol will slash their GGP collateral to reward liquid stakers. This is also calculated using the price of GGP - AVAX:

File: MinipoolManager.sol

547:	function calculateGGPSlashAmt(uint256 avaxRewardAmt) public view returns (uint256) {
548:		Oracle oracle = Oracle(getContractAddress("Oracle"));
549:		(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX(); // price might change or be manipulated
550:		return avaxRewardAmt.divWadDown(ggpPriceInAvax);
551:	}

...

670:	function slash(int256 index) private {

...

673:		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
674:		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
675:		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
676:		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

...

681:		Staking staking = Staking(getContractAddress("Staking"));
682:		staking.slashGGP(owner, slashGGPAmt);
683:	}

This is then subtracted from their staked amount:

File: Staking.sol

94: 	function decreaseGGPStake(address stakerAddr, uint256 amount) internal {
95: 		int256 stakerIndex = requireValidStaker(stakerAddr);
96: 		subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount); // can fail due to underflow
97: 	}

...

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

The issue is that the current staked amount is never checked so the subUint can fail due to underflow if the price has changed since the minipool was created/recreated.

Impact

If a node operator doesn't have enough collateral, possibly caused by price changes in GGP during slashing they evade slashing all together.

Its even possible for the node operator to foresee this and manipulate the price of GGP just prior to the period ending if they know that they are going to be slashed.

Proof of Concept

PoC test in MinipoolManager.t.sol:

	function testRecordStakingEndWithSlashNotEnoughStake() public {
		uint256 duration = 365 days;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether; // just enough

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

		skip(2 weeks);
		
		vm.prank(address(rialto)); // price changes just a bit
		oracle.setGGPPriceInAVAX(0.999 ether, block.timestamp);

		vm.prank(address(rialto));
		vm.expectRevert(); // staking cannot end because of underflow
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
	}

The only thing the protocol can do now is to call recordStakingError for the minipool, since no other state changes are allowed. This will return the staked funds but it will not slash the GGP amount for the node operator. Hence the node operator has evaded the slashing.

Tools Used

vs code, forge

Recommended Mitigation Steps

If the amount to be slashed is greater than what the node operator has staked, slash all their stake.

Lines of code

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

Vulnerability details

Description

When a node operator creates a minipool they pass which duration they want to stake for. There is no validation for this field so they can pass any field:

File: MinipoolManager.sol

196:	function createMinipool(
197:		address nodeID,
198:		uint256 duration,
199:		uint256 delegationFee,
200:		uint256 avaxAssignmentRequest
201:	) external payable whenNotPaused {

...     // no validation for duration

256:		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));
257:		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration); // duration stored
258:		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".delegationFee")), delegationFee);

Later when staking is done. if the node op was slashed, duration is used to calculate the slashing amount:

File: MinipoolManager.sol

557:	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
558:		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
559:		uint256 rate = dao.getExpectedAVAXRewardsRate();
560:		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
561:	}

...

670:	function slash(int256 index) private {

...

673:		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
674:		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
675:		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
676:		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

The node operator cannot pass in 0 because that reverts due to zero transfer check in Vault. However the node operator can pass in 1 to guarantee the lowest slash amount possible.

Rialto might fail this, but there is little information about how Rialto uses the duration passed. According to this comment they might default to 14 days in which this finding is valid:

JohnnyGault — 12/30/2022 3:22 PM

To clarify duration for everyone -- a nodeOp can choose a duration they want, from 14 days to 365 days. But behind the scenes, Rialto will only create a validator for 14 days. ...

Impact

The node operator can send in a very low duration to get minimize slashing amounts. It depends on the implementation in Rialto, which we cannot see. Hence submitting this.

Proof of Concept

PoC test in MinipoolManager.t.sol:

	function testRecordStakingEndWithSlashZeroDuration() public {
		uint256 duration = 1; // zero duration causes vault to fail on 0 amount
		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();

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

		skip(2 weeks);
		
		vm.prank(address(rialto));
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

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

		int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
		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);

		// very small slash amount
		assertLt(mp1Updated.ggpSlashAmt, 0.000_01 ether);
		assertGt(staking.getGGPStake(mp1Updated.owner), ggpStakeAmt - 0.000_01 ether);
	}

Tools Used

vs code, forge

Recommended Mitigation Steps

Regardless if Rialto will fail this or not, I recommend that the duration passed is validated to be withing 14 days and 365 days.