Submitted by HollaDieWaldfee, also found by enckrish, imare, bin2chen, danyams, 0xdeadbeef0x, cozzetti, and ladboy233
When staking is done, a Rialto multisig calls MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440).
If the avaxTotalRewardAmt has the value zero, the MinipoolManager will slash the node operators GGP.
The issue is that the amount to slash can be greater than the GGP balance the node operator has staked.
This will cause the call to MinipoolManager.recordStakingEnd to revert because an underflow is detected.
This means a node operator can create a minipool that cannot be slashed.
A node operator must provide at least 10% of avaxAssigned as collateral by staking GGP.
It is assumed that a node operator earns AVAX at a rate of 10% per year.
So if a Minipool is created with a duration of > 365 days, the 10% collateral is not sufficient to pay the expected rewards.
This causes the function call to revert.
Another cause of the revert can be that the GGP price in AVAX changes. Specifically if the GGP price falls, there needs to be slashed more GGP.
Therefore if the GGP price drops enough it can cause the call to slash to revert.
I think it is important to say that with any collateralization ratio this can happen. The price of GGP must just drop enough or one must use a long enough duration.
The exact impact of this also depends on how the Rialto multisig handles failed calls to MinipoolManager.recordStakingEnd.
It looks like if this happens, MinipoolManager.recordStakingError (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515) is called.
This allows the node operator to withdraw his GGP stake.
So in summary a node operator can create a Minipool that cannot be slashed and probably remove his GGP stake when it should have been slashed.
When calling MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440) and the avaxTotalRewardAmt parameter is zero, the node operator is slashed:
The MinipoolManager.slash function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670-L683) then calculates expectedAVAXRewardsAmt and from this slashGGPAmt:
Downstream there is then a revert due to underflow because of the following line in Staking.decreaseGGPStake (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97):
You can add the following foundry test to MinipoolManager.t.sol:
See that it runs successfully with duration = 365 days and fails with duration = 366 days.
The similar issue occurs when the GGP price drops. I chose to implement the test with duration as the cause for the underflow because your tests use a fixed AVAX/GGP price.
VSCode, Foundry
You should check if the amount to be slashed is greater than the node operators GGP balance. If this is the case, the amount to be slashed should be set to the node operators GGP balance.
I believe this check can be implemented within the MinipoolManager.slash function without breaking any of the existing accounting logic.
emersoncloud (GoGoPool) confirmed, but commented:
Alex the Entreprenerd (judge) commented:
Alex the Entreprenerd (judge) commented:
emersoncloud (GoGoPool) mitigated:
Status: Original finding mitigated, but a medium severity economical risk is still present. Full details in reports from RaymondFam, ladboy233 and hansfriese. Also included in Mitigation Review section below.
