dos to recordStakeEnd and slash in MinipoolManager

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L385-L440
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L424-L426
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L670-L683
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L379-L383
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L94-L97
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/BaseAbstract.sol#L195-L197
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Storage.sol#L176-L178

Vulnerability details

Impact

ggp is staked by node operator as a collateral for liquid stakers avax. However, there are conditions whereby slash will revert and therefore causing recordStakeEnd to revert too in the event that rialto wants to slash the node operator for bad behaviour. The funds will be stuck and there is no way forward unless rialto decides to reward node operator even though they are meant to be slashed.

Proof of Concept

When rialto decides to slash a node operator, they call recordStakingEnd with 0 avaxTotalRewardAmt. This will call slash which will call staking's slashGGP. Order of function calls, recordStakingEnd -> slash -> slashGGP.

		if (avaxTotalRewardAmt == 0) {
			slash(minipoolIndex);
		}

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

However, notice slashGGP, it decreaseGGPStake directly from stakerAddr.

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

	function decreaseGGPStake(address stakerAddr, uint256 amount) internal {
		int256 stakerIndex = requireValidStaker(stakerAddr);
		subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);
	}

	function subUint(bytes32 key, uint256 amount) internal {
		gogoStorage.subUint(key, amount);
	}

	function subUint(bytes32 key, uint256 amount) external onlyRegisteredNetworkContract {
		uintStorage[key] = uintStorage[key] - amount;
	}

This will result in a revert if stakedGGP for staker is less than expectedAVAXRewardsAmt due to underflow for the staker.item storage. Below are conditions that might result in a revert.

1. drop in GGP price
2. rise in AVAX price
3. 365 days duration

This proof of concept in MinipoolManager.t.sol shows a 12 month validation period which the protocol incentivizes node operator to sign up for according to the docs. By just dropping the price of ggp in avax from 1 ether to 0.9 ether, the staked GGP is insufficient in covering the expectedAVAXRewardsAmt and therefore causes dos to recordStakeEnd with slash reverting.

@@ -434,11 +434,11 @@ contract MinipoolManagerTest is BaseTest {
        }

        function testRecordStakingEndWithSlash() public {
-               uint256 duration = 2 weeks;
+               uint256 duration = 365 days;
                uint256 depositAmt = 1000 ether;
                uint256 avaxAssignmentRequest = 1000 ether;
                uint256 validationAmt = depositAmt + avaxAssignmentRequest;
-               uint128 ggpStakeAmt = 200 ether;
+               uint128 ggpStakeAmt = 100 ether;

                vm.startPrank(nodeOp);
                ggp.approve(address(staking), MAX_AMT);
@@ -481,6 +481,7 @@ contract MinipoolManagerTest is BaseTest {
                vm.expectRevert(MinipoolManager.InvalidAmount.selector);
                minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 9 ether);

+               oracle.setGGPPriceInAVAX(0.9 ether, block.timestamp);
                minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

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

Tools Used

Foundry

Recommended Mitigation Steps

Recommend making ggp staked liquidable so that it can be liquidated in the event ggp staked dropped below expectedAVAXRewardsAmt and also making ggp staked amount required based on the duration of days staked.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #136

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[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