Users should still be slashed the total amount of their GGP staked even if they do not meet the calculated amount

[code423n4]

Lines of code

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

Vulnerability details

Impact

Currently, slash() will revert if the calculated amount of staked GGP is more than node operator's staked amount as they do not have enough to pay for it. This is problematic as in such situations, the function will revert instead of simply slashing all of their staked amount.

Proof of Concept

Currently, there is a 10% requirement of GGP staked in order for a node operator to create a minipool. This is used as a precaution to compensate liquid stakers if node operator does not fufill its obligations. However, there is no guarantee that this amount is sufficient.

Slash amount is calculated based on the expected AVAX reward amount. It might be possible for this amount to be higher than the minimum GGP staked amount.

MinipoolManager.sol#L670-L683

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

When we slash a user, we are subtracting their current staked GGP with slashGGPAmt. This can underflow and revert the slash function if they do not have enough.

Tools Used

Manual Review

Recommended Mitigation Steps

We have a simple and effective fix for this.

We can slash the minimum of slashGGPAmt and staker's GGP stake instead.

- staking.slashGGP(owner, slashGGPAmt);
+ staking.slashGGP(owner, min(slashGGPAmt, getGGPStake(stakerAddr))); 

[GalloDaSballo]

Despite the nuance I think this ultimately is a dup of the reports using a longer duration to cause an overflow and prevent slashing

[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 changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #494

[c4-judge]

GalloDaSballo marked the issue as satisfactory