the lack of checks for Minipool duration which can cause multiple issues like penalty bypass or fund loss

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L191-L269
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L380-L440
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L442-L478
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L667-L683
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L553-L561

Vulnerability details

Impact

duration of the Minipool is specified by node runner and it should be between 14 days to 356 days and Multisig should recreate Minipool at 14 days long so that total duration would be covered and if node runner fails the validation then contract would slash node runner and distributes his ggp collaterals between liquidity staker. in current implementation there is not enough checks and validation for duration of the Minipools to ensure all the features based on Minipool's duration the impacts are:

1. there is no check that duration of the Minipool is less than 365 days and if user by mistake set very high value for duration and fails to run node properly user would lose very large number of his GGP collaterals because the dominator is 365 days when calculating slash amount and this would cause node runner to loss more funds.
2. there is no check that duration of the Minipool is bigger than 14 days and a malicious node runner can set duration as 0 day and if he fails the calculated slash amount would be 0.
3. there is no check that make sure Multisig don't recreate Minipool more than Minipool duration and if Multisig recreates Minipool by mistake or intentionally then node runner funds would stuck more than he wanted and if node runner fail to run node properly in extra Minipool cycles (because he didn't plan to run node for those extra times) then code would slash his collateral for all duration time.
4. when node runner fails to run node properly he would slashed based on total duration of the Minipool but the fail maybe happens in the last cycles of Minipool and code don't consider the valid time node runner was performed and calculate slash for total duration. this would cause node runners to lose a lot of funds even if they fail in the last round of the Minipool cycle.

Proof of Concept

This is createMinipool() code:

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
		if (nodeID == address(0)) {
			revert InvalidNodeID();
		}

		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		if (
			// Current rule is matched funds must be 1:1 nodeOp:LiqStaker
			msg.value != avaxAssignmentRequest ||
			avaxAssignmentRequest > dao.getMinipoolMaxAVAXAssignment() ||
			avaxAssignmentRequest < dao.getMinipoolMinAVAXAssignment()
		) {
			revert InvalidAVAXAssignmentRequest();
		}

		if (msg.value + avaxAssignmentRequest < dao.getMinipoolMinAVAXStakingAmt()) {
			revert InsufficientAVAXForMinipoolCreation();
		}

		Staking staking = Staking(getContractAddress("Staking"));
		staking.increaseMinipoolCount(msg.sender);
		staking.increaseAVAXStake(msg.sender, msg.value);
		staking.increaseAVAXAssigned(msg.sender, avaxAssignmentRequest);

		if (staking.getRewardsStartTime(msg.sender) == 0) {
			staking.setRewardsStartTime(msg.sender, block.timestamp);
		}

		uint256 ratio = staking.getCollateralizationRatio(msg.sender);
		if (ratio < dao.getMinCollateralizationRatio()) {
			revert InsufficientGGPCollateralization();
		}

		// Get a Rialto multisig to assign for this minipool
		MultisigManager multisigManager = MultisigManager(getContractAddress("MultisigManager"));
		address multisig = multisigManager.requireNextActiveMultisig();
......
......
	}

As you can see there is no check that duration is between 14 days and 365 days and it's possible to create Minipools with duration as 0 and Minipools with duration as very big number. This is slash() and getExpectedAVAXRewardsAmt() code:

	function slash(int256 index) private {
		address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID")));
		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);

		emit GGPSlashed(nodeID, slashGGPAmt);

		Staking staking = Staking(getContractAddress("Staking"));
		staking.slashGGP(owner, slashGGPAmt);
	}

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

As you can see the slash amount is calculated based on Minipool's duration ((avaxAmt * duration) / (rate * 365)) .

• if a malicious node runner set duration as 0 then whenever he fails to run validation node properly code would slash 0 tokens (assuming Multisig runs the Minipool regardless of 0 day duration) and node runner can bypass slashing mechanism.
• if a node runner set very high value for duration like 10000 days then code would accept it and whenever he fails to run validation node properly then code slash node runner for very high amount because of high value of duration.
• If a node runner run his node for almost all of the duration time correctly and in the last cycle he fails to run it then Multisig would call recordStakingEnd() with 0 rewards and node runner would get slashed for whole duration while node runner generated validation reward for most of the duration time.
  This is the recreateMinipool() code:

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

		Staking staking = Staking(getContractAddress("Staking"));
		// Only increase AVAX stake by rewards amount we are compounding
		// since AVAX stake is only decreased by withdrawMinipool()
		staking.increaseAVAXStake(mp.owner, mp.avaxNodeOpRewardAmt);
		staking.increaseAVAXAssigned(mp.owner, compoundedAvaxNodeOpAmt);
		staking.increaseMinipoolCount(mp.owner);

		if (staking.getRewardsStartTime(mp.owner) == 0) {
			// Edge case where calculateAndDistributeRewards has reset their rewards time even though they are still cycling
			// So we re-set it here to their initial start time for this minipool
			staking.setRewardsStartTime(mp.owner, mp.initialStartTime);
		}

		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 ratio = staking.getCollateralizationRatio(mp.owner);
		if (ratio < dao.getMinCollateralizationRatio()) {
			revert InsufficientGGPCollateralization();
		}

		resetMinipoolData(minipoolIndex);

		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch);
	}

As you can see there is no check that Multisig is not over recycling Minipool and if a user creates a validation node (prepare hardware) for specific amount of time for example 140 days and create a Minipool for 140 days for his node and he expects that Minipool would end after 140 days but there is no logic in the code to ensure this and Multisig can recreate Minipool and the real duration of the Minipool can extend the 140 days and because node runner hardware won't be available more than 140 days so node runner would fail to validate and generate reward in the last cycle and he would be slashed for the whole duration while node runner had valid node for the entire duration. code should have some checks and make sure Multisig won't over recreate Minipool.

Tools Used

VIM

Recommended Mitigation Steps

add more checks for duration value and also fix the logic of slash() to only slash time node didn't work properly and change recreateMinipool() to make sure Multisig won't over recreate Minipool.

[GalloDaSballo]

Primary because it groups findings, but I think the warden should have separated the Slash grief vs the Duration check

[c4-judge]

GalloDaSballo marked the issue as primary issue

[emersoncloud]

Agreed on the separation of slash grief vs duration check.

1, 2 and 4 will be addressed with a better duration check during the slash calculation. I'll mark this as a duplicate and link to the other issue when I find it.

3. Out of scope for this audit, as it relies on a privileged actor misbehaving. From our audit parameters:

There are several privileged actors in the protocol (Rialto TSS Multisig, and Guardian Multisig) and contract functions that are only callable by these privileged actors. If there is an exploit which can only be executed by these privileged actors we do not consider that to be in scope for this audit.

[c4-sponsor]

emersoncloud marked the issue as disagree with severity

[0xju1ie]

Impact 1 & 2 is not valid as it would result in recordStakingError()

[0xju1ie]

4. when node runner fails to run node properly he would slashed based on total duration of the Minipool but the fail maybe happens in the last cycles of Minipool and code don't consider the valid time node runner was performed and calculate slash for total duration. this would cause node runners to lose a lot of funds even if they fail in the last round of the Minipool cycle.

Only valid impact for this issue

[0xju1ie]

duplicate of #493

[GalloDaSballo]

In contrast to other reports, this report shows:

• Slashing skip if 0 amount (FSM issue, dup of any duration can be passed by node operator #492)
• Slashed for Full Duration (node operator is getting slashed for full duration even though rewards are distributed based on a 14 day cycle #493)

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

Duplicate of #533

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Closing after having split this into 2