Upgraded Q -> 2 from #800 [1675429611496]

[c4-judge]

Judge has assessed an item in Issue #800 as 2 risk. The relevant finding follows:

1- recordStakingError function doesn't decrease the minipool avaxLiquidStakerAmt value : When the function recordStakingError is called by the multisig it decreases both the total AVAX staking amount and the AVAX assigned to the minipool owner, but it does not reset the minipool avaxLiquidStakerAmt back to zero and leaves it equal to the previous amount. In the current contract logic this doesn't have a big impact (only have an impact in the view functions : getMinipool, getMinipools as they will return wrong information) but as the contract is meant to be upgradable it can cause damage in the future implementations.

Risk : Low Proof of Concept Issue occurs in the instance below : File: contracts/contract/MinipoolManager.sol Line 484-515

function recordStakingError(address nodeID, bytes32 errorCode) external payable { int256 minipoolIndex = onlyValidMultisig(nodeID); requireValidStateTransition(minipoolIndex, MinipoolStatus.Error); address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); if (msg.value != (avaxNodeOpAmt + avaxLiquidStakerAmt)) { revert InvalidAmount(); } setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode); setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Error)); setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxTotalRewardAmt")), 0); setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")), 0); setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerRewardAmt")), 0); // Send the nodeOps AVAX to vault so they can claim later Vault vault = Vault(getContractAddress("Vault")); vault.depositAVAX{value: avaxNodeOpAmt}(); // Return Liq stakers funds ggAVAX.depositFromStaking{value: avaxLiquidStakerAmt}(avaxLiquidStakerAmt, 0); /* @audit In the lines below the AVAX assigned to the owner & TotalAVAXLiquidStakerAmt are updated But the minipool.avaxLiquidStakerAmt is not reset to zero / Staking staking = Staking(getContractAddress("Staking")); staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt); subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt); emit MinipoolStatusChanged(nodeID, MinipoolStatus.Error); } As you can see from the code above the value of minipool.avaxLiquidStakerAmt is not updated and will remain the same until the minipool is recreated, this will return wrong minipool information when both the functions getMinipool and getMinipools are called and it can also cause problems if the contract logic is upgraded in the future and uses this wrong value.

Mitigation To avoid this issue i recommend to reset the value minipool.avaxLiquidStakerAmt back to zero and the recordStakingError function should be modified as follow :

function recordStakingError(address nodeID, bytes32 errorCode) external payable { int256 minipoolIndex = onlyValidMultisig(nodeID); requireValidStateTransition(minipoolIndex, MinipoolStatus.Error); address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner"))); uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt"))); uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt"))); ... Staking staking = Staking(getContractAddress("Staking")); staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt); subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt); /* @audit Reset minipool.avaxLiquidStakerAmt back to zero / setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), 0); emit MinipoolStatusChanged(nodeID, MinipoolStatus.Error);

[c4-judge]

GalloDaSballo marked the issue as duplicate of #569

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as partial-50

[c4-judge]

This auto-generated issue was withdrawn by GalloDaSballo

[Simon-Busch]

Changed severity back to M as requested by @GalloDaSballo