It might be impossible to slash a minipool

[code423n4]

Lines of code

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

Vulnerability details

Impact

The slashing functionality is mostly implemented in the private slash function of the MinipoolManager contract. The amount to be slashed is calculated calling the calculateGGPSlashAmt function. The amount returned by this function is finally taken from the amount of GGP staked by the node within the slashGGP function of the Staking contract.

However, the code never checks whether the node has sufficient GGP staked to cover the slashed amount. It simply attempts to slash the full amount calculated by calculateGGPSlashAmt. If there's not enough GGP at stake available to cover the full amount, the call to the decreaseGGPStake function will revert with an arithmetic error, and the whole slashing operation will be reverted.

Because the value returned by calculateGGPSlashAmt depends on the price of GGP in AVAX, when the price of GGP drops, more GGP will be needed to cover the entire slashing. Therefore, any drop in the price of GGP in AVAX might trigger this scenario. It will also necessarily depend on the amount of GGP staked by a node operator.

Proof of Concept

Paste the following Foundry test in the MinipoolManager.t.sol file:

function testSlashingRevertsWithArithmeticError() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmount = depositAmt + avaxAssignmentRequest;
		uint128 initialGGPStakeAmount = 100 ether;

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

		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

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

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

		skip(duration);

		// GGP Price in AVAX drops
		oracle.setGGPPriceInAVAX(0.035 ether, block.timestamp);

		// There won't be enough GGP to cover the full slashing amount
		// so slashing becomes impossible
		vm.expectRevert(stdError.arithmeticError);
		minipoolMgr.recordStakingEnd{value: validationAmount}(
			mp1.nodeID,
			block.timestamp,
			0 ether // to trigger slashing of GGP
		);
		vm.stopPrank();
	}

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

The case where there's not enough GGP at stake to cover the full slashing amount must be explicitly handled. There's currently no documentation, tests nor a specification that states what is the expected behavior of the system in this scenario. One possible mitigation might be to slash the available amount of GGP stake when the full amount cannot be covered. The staker would still owe GGP to the protocol, but it could be a better scenario than not slashing at all.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #136

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo changed the severity to 3 (High Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #494

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory