GoGoPool contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Incorrect validation of node status when handling existing id when creating MiniPool

Proof of Concept

Let us look into the function createMiniPool:

/// @notice Accept AVAX deposit from node operator to create a Minipool. Node Operator must be staking GGP. Open to public.
/// @param nodeID 20-byte Avalanche node ID
/// @param duration Requested validation period in seconds
/// @param delegationFee Percentage delegation fee in units of ether (2% is 0.2 ether)
/// @param avaxAssignmentRequest Amount of requested AVAX to be matched for this Minipool
function createMinipool(
	address nodeID,
	uint256 duration,
	uint256 delegationFee,
	uint256 avaxAssignmentRequest
) external payable whenNotPaused {

which calls:

// Create or update a minipool record for nodeID
// If nodeID exists, only allow overwriting if node is finished or canceled
// 		(completed its validation period and all rewards paid and processing is complete)
int256 minipoolIndex = getIndexOf(nodeID);
if (minipoolIndex != -1) {
	requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
	resetMinipoolData(minipoolIndex);
	// Also reset initialStartTime as we are starting a whole new validation
	setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
} else {
	minipoolIndex = int256(getUint(keccak256("minipool.count")));
	// The minipoolIndex is stored 1 greater than actual value. The 1 is subtracted in getIndexOf()
	setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1));
	setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID);
	addUint(keccak256("minipool.count"), 1);
}

node the comment:

If nodeID exists, only allow overwriting if node is finished or canceled

However, in the code

requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);

which calls:

function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
	bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
	MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
	bool isValid;

	if (currentStatus == MinipoolStatus.Prelaunch) {
		isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
	}

note the check:

isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);

the intention is to check if the node status is finished or canceled, but the implementation check if the node status is Launched or canceled.

A launched node status should not be overwritten.

Tools Used

Manual Review

Recommended Mitigation Steps

Change from

if (currentStatus == MinipoolStatus.Prelaunch) {
	isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);

to

if (currentStatus == MinipoolStatus.Prelaunch) {
	isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Canceled);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L169 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/lib/solmate/src/mixins/ERC4626.sol#L137 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/lib/solmate/src/mixins/ERC4626.sol#L127

Vulnerability details

Impact

A malicious early user/attacker can manipulate the pricePerShare in ggAVAX token vault to take an unfair share of future users' deposits

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Proof of Concept

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the token, and get 1 wei of shares.

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L34 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L198 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L257 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L675 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L560 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L379 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94

Vulnerability details

Impact

Node creator can game the duration parameter to trigger underflow when slashing is called.

Proof of Concept

When miniPool is created, this function called:

/// @notice Accept AVAX deposit from node operator to create a Minipool. Node Operator must be staking GGP. Open to public.
/// @param nodeID 20-byte Avalanche node ID
/// @param duration Requested validation period in seconds
/// @param delegationFee Percentage delegation fee in units of ether (2% is 0.2 ether)
/// @param avaxAssignmentRequest Amount of requested AVAX to be matched for this Minipool
function createMinipool(
	address nodeID,
	uint256 duration,
	uint256 delegationFee,
	uint256 avaxAssignmentRequest
) external payable whenNotPaused {

note the parameter duration: duration

There is no boundry check about this parameter, the user can pass in the duration whatever value they want.

In the begining, this parameter is recorded:

setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration);

This parameter is used when need to calculate the slash calculation:

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

note the code:

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

note the function

/// @notice Given a duration and an AVAX amt, calculate how much AVAX should be earned via validation rewards
/// @param duration The length of validation in seconds
/// @param avaxAmt The amount of AVAX the node staked for their validation period
/// @return The approximate rewards the node should recieve from Avalanche for beign a validator
function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
	ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
	uint256 rate = dao.getExpectedAVAXRewardsRate();
	return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
}

the longer the duration is, the larger the slashed amount is, once the slashed amount is larger than the staked GGP token amount, arithmic underflow happens.

for example, user stake 100 GGP token, but because duration is long, slashed amount goes to 120 GGP, slash function will revert.

then We call

staking.slashGGP(owner, slashGGPAmt);

which calls:

/// @notice Minipool Manager will call this if a minipool ended and was not in good standing
/// @param stakerAddr The C-chain address of a GGP staker in the protocol
/// @param ggpAmt The amount of GGP being slashed
function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
	Vault vault = Vault(getContractAddress("Vault"));
	decreaseGGPStake(stakerAddr, ggpAmt);
	vault.transferToken("ProtocolDAO", ggp, ggpAmt);
}

which calls:

/// @notice Decrease the amount of GGP a given staker is staking
/// @param stakerAddr The C-chain address of a GGP staker in the protocol
function decreaseGGPStake(address stakerAddr, uint256 amount) internal {
	int256 stakerIndex = requireValidStaker(stakerAddr);
	subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);
}

which calls subUint

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

which finally calls:

/// @param key The key for the record
/// @param amount An amount to subtract from the record's value
function subUint(bytes32 key, uint256 amount) external onlyRegisteredNetworkContract {
	uintStorage[key] = uintStorage[key] - amount;
}

basically, a inflated duration setting can trigger arithmic underflow in

uintStorage[key] - amount

it is the staked GGP token from the node creator get slashed.

Now we need to see the POC in action:

In MiniPoolManager.t.sol

in test testRecordStakingEndWithSlash

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L436

the normal duration parameter is 2 weeks, if we comment out the 2 weeks and change the duration to 200 weeks

function testRecordStakingEndWithSlash() public {
	// uint256 duration = 2 weeks;
	uint256 duration = 200 weeks;

We run the test

forge test -vvv --match testRecordStakingEndWithSlash

the transaction revert in arithmic underflow

    │   ├─ [6811] Staking::slashGGP(nodeOp: [0x0000000000000000000000000000000000050001], 383561643835616438356)
    │   │   ├─ [537] Storage::getAddress(0x817c4b3de237861e6f469d75ed1cda82d8bdda3182404c90d5f1beeef56e7190) [staticcall]
    │   │   │   └─ ← MinipoolManager: [0xdC0505A4c93647CD736E4bE24FBb171472bBFb10]
    │   │   ├─ [537] Storage::getAddress(0xcbd1c5b0c4d42cdfd729ee182f5c8992c7d923b770e4f43f682303afa9a8eb08) [staticcall]
    │   │   │   └─ ← Vault: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382]
    │   │   ├─ [549] Storage::getUint(0xc8d6547c79d121d9c9e35f4887cbaef36c2766533faf1b1ef5b0898f0e91a456) [staticcall]
    │   │   │   └─ ← 1
    │   │   ├─ [1016] Storage::subUint(0x1328d42ca0068ee293c0b73399873fd8b26ccb19d4419e75957733832b51665a, 383561643835616438356)
    │   │   │   └─ ← "Arithmetic over/underflow"
    │   │   └─ ← "Arithmetic over/underflow"
    │   └─ ← "Arithmetic over/underflow"
    └─ ← "Arithmetic over/underflow"

Test result: FAILED. 0 passed; 1 failed; finished in 12.16ms

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add reasonable boundary check and if the slashed amount is larger than the avaiable staked GGP token, slash all GGP + part of AVAX from node operator if possible to handle this case gracefully instead of triggering arithmic underflow.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L328 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L89

Vulnerability details

Impact

Missing WhenNotPaused modifier for restakeGGP in Staking.sol

Proof of Concept

The admin can pause a contract in urgent sitation or the governance can pause a contract as they see fit.

/// @dev Verify contract is not paused
modifier whenNotPaused() {
	string memory contractName = getContractName(address(this));
	if (getBool(keccak256(abi.encodePacked("contract.paused", contractName)))) {
		revert ContractPaused();
	}
	_;
}

As we can see in the code, when the contract is paused, the stakeGGP is blocked in Staking.sol

/// @notice Accept a GGP stake
/// @param amount The amount of GGP being staked
function stakeGGP(uint256 amount) external whenNotPaused {
	// Transfer GGP tokens from staker to this contract
	ggp.safeTransferFrom(msg.sender, address(this), amount);
	_stakeGGP(msg.sender, amount);
}

but the restakeGGP function is missing WhenNotPaused.

/// @notice Convenience function to allow for restaking claimed GGP rewards
/// @param stakerAddr The C-chain address of a GGP staker in the protocol
/// @param amount The amount of GGP being staked
function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
	// Transfer GGP tokens from the ClaimNodeOp contract to this contract
	ggp.safeTransferFrom(msg.sender, address(this), amount);
	_stakeGGP(stakerAddr, amount);
}

Use can bypass the WhenNotPaused modifier by calling ClaimNodeOp#claimAndRestake

	/// @notice Claim GGP and automatically restake the remaining unclaimed rewards
	/// @param claimAmt The amount of GGP the staker would like to withdraw from the protocol
	function claimAndRestake(uint256 claimAmt) external {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 ggpRewards = staking.getGGPRewards(msg.sender);
		if (ggpRewards == 0) {
			revert NoRewardsToClaim();
		}
		if (claimAmt > ggpRewards) {
			revert InvalidAmount();
		}

		staking.decreaseGGPRewards(msg.sender, ggpRewards);

		Vault vault = Vault(getContractAddress("Vault"));
		uint256 restakeAmt = ggpRewards - claimAmt;
		if (restakeAmt > 0) {
			vault.withdrawToken(address(this), ggp, restakeAmt);
			ggp.approve(address(staking), restakeAmt);
			staking.restakeGGP(msg.sender, restakeAmt);
		}

		if (claimAmt > 0) {
			vault.withdrawToken(msg.sender, ggp, claimAmt);
		}

		emit GGPRewardsClaimed(msg.sender, claimAmt);
	}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add the WhenNotPaused modifier to restakeGGP to not let user bypass the paused state

Lines of code

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

Vulnerability details

Impact

The multisig wallet owner is not able to cancel a miniPool

Proof of Concept

	/// @notice Multisig can cancel a minipool if a problem was encountered *before* claimAndInitiateStaking() was called
	/// @param nodeID 20-byte Avalanche node ID
	/// @param errorCode The code that represents the reason for failure
	function cancelMinipoolByMultisig(address nodeID, bytes32 errorCode) external {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode);
		_cancelMinipoolAndReturnFunds(nodeID, minipoolIndex);
	}

which calls

	/// @notice Cancels the minipool and returns the funds related to it
	/// @dev At this point we dont have any liq staker funds withdrawn from ggAVAX so no need to return them
	/// @param nodeID 20-byte Avalanche node ID
	/// @param index Index of the minipool
	function _cancelMinipoolAndReturnFunds(address nodeID, int256 index) private {
		requireValidStateTransition(index, MinipoolStatus.Canceled);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".status")), uint256(MinipoolStatus.Canceled));

		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
		uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpAmt")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));

		Staking staking = Staking(getContractAddress("Staking"));
		staking.decreaseAVAXStake(owner, avaxNodeOpAmt);
		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);

		staking.decreaseMinipoolCount(owner);

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Canceled);

		Vault vault = Vault(getContractAddress("Vault"));
		vault.withdrawAVAX(avaxNodeOpAmt);
		owner.safeTransferETH(avaxNodeOpAmt);
	}

the owner is the creator of the miniPool, it could not only be a EOA account,

but also a smart contnract.

safeTransferETH assume that receiver must receive the ETH (AVAX). However, the owner could be a smart contract intentionally or intentionally that does not implement receive fallback function and does not accept ETH (AVAX), then the transaction revert and the caller is not able to complete the transaction cancelMinipoolByMultisig

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project let the owner of the miniPool claim the AVAX instead of sending the ETH out to them otherwise they could block the AVAX and revert the cancelMinipoolByMultisig transaction.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L229 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L156

Vulnerability details

Impact

Division by zero error can block RewardsPool#startRewardCycle if all multisig wallet is disabled.

Proof of Concept

A user needs to call the function startRewardsCycle in RewardsPool.sol

/// @notice Public function that will run a GGP rewards cycle if possible
function startRewardsCycle() external {

which calls:

uint256 multisigClaimContractAllotment = getClaimingContractDistribution("ClaimMultisig");
uint256 nopClaimContractAllotment = getClaimingContractDistribution("ClaimNodeOp");
uint256 daoClaimContractAllotment = getClaimingContractDistribution("ClaimProtocolDAO");
if (daoClaimContractAllotment + nopClaimContractAllotment + multisigClaimContractAllotment > getRewardsCycleTotalAmt()) {
	revert IncorrectRewardsDistribution();
}

TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
Vault vault = Vault(getContractAddress("Vault"));

if (daoClaimContractAllotment > 0) {
	emit ProtocolDAORewardsTransfered(daoClaimContractAllotment);
	vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment);
}

if (multisigClaimContractAllotment > 0) {
	emit MultisigRewardsTransfered(multisigClaimContractAllotment);
	distributeMultisigAllotment(multisigClaimContractAllotment, vault, ggp);
}

if (nopClaimContractAllotment > 0) {
	emit ClaimNodeOpRewardsTransfered(nopClaimContractAllotment);
	ClaimNodeOp nopClaim = ClaimNodeOp(getContractAddress("ClaimNodeOp"));
	nopClaim.setRewardsCycleTotal(nopClaimContractAllotment);
	vault.transferToken("ClaimNodeOp", ggp, nopClaimContractAllotment);
}

We need to pay speical attention to the code block below:

if (multisigClaimContractAllotment > 0) {
	emit MultisigRewardsTransfered(multisigClaimContractAllotment);
	distributeMultisigAllotment(multisigClaimContractAllotment, vault, ggp);
}

which calls:

/// @notice Distributes GGP to enabled Multisigs
/// @param allotment Total GGP for Multisigs
/// @param vault Vault contract
/// @param ggp TokenGGP contract
function distributeMultisigAllotment(
uint256 allotment,
Vault vault,
TokenGGP ggp
) internal {
MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));

uint256 enabledCount;
uint256 count = mm.getCount();
address[] memory enabledMultisigs = new address[](count);

// there should never be more than a few multisigs, so a loop should be fine here
for (uint256 i = 0; i < count; i++) {
	(address addr, bool enabled) = mm.getMultisig(i);
	if (enabled) {
		enabledMultisigs[enabledCount] = addr;
		enabledCount++;
	}
}

// Dirty hack to cut unused elements off end of return value (from RP)
// solhint-disable-next-line no-inline-assembly
assembly {
	mstore(enabledMultisigs, enabledCount)
}

uint256 tokensPerMultisig = allotment / enabledCount;
for (uint256 i = 0; i < enabledMultisigs.length; i++) {
	vault.withdrawToken(enabledMultisigs[i], ggp, tokensPerMultisig);
}
}

the code distribute the reward to all multisig evenly.

uint256 tokensPerMultisig = allotment / enabledCount;

However, if the enabledCount is 0, meaning no multisig wallet is enabled, the transactionr revert in division by zero error and revert the startRewardsCycle transaction.

As showns in POC.

In RewardsPool.t.sol,

we change the name from testStartRewardsCycle to testStartRewardsCycle_POC

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/RewardsPool.t.sol#L123

we add the code to disable all multisig wallet. before calling rewardsPool.startRewardsCycle

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/RewardsPool.t.sol#L138

// disable all multisg wallet
vm.prank(guardian);
ocyticus.disableAllMultisigs();

Then we run the test

forge test -vvv --match testStartRewardsCycle_POC

the transaction revert in division by zero error, which block the startRewardsCycle

    │   ├─ emit MultisigRewardsTransfered(value: 13499352589262561353689)
    │   ├─ [537] Storage::getAddress(0xcda836d09bcf3adcec2f52ddddeceac31738a574d5063511c887064e499593df) [staticcall]
    │   │   └─ ← MultisigManager: [0xA12E9172eB5A8B9054F897cC231Cd7a2751D6D93]
    │   ├─ [1313] MultisigManager::getCount() [staticcall]
    │   │   ├─ [549] Storage::getUint(0x778484468bc504108f077f6bf471293e4138c2d117c6f33607855518cf4bda79) [staticcall]
    │   │   │   └─ ← 1
    │   │   └─ ← 1
    │   ├─ [3050] MultisigManager::getMultisig(0) [staticcall]
    │   │   ├─ [537] Storage::getAddress(0xfebe6f39b65f18e050b53df1d0c8d45b8c5cce333324eb048b67b8ee5f26b7a3) [staticcall]
    │   │   │   └─ ← RialtoSimulator: [0x98D1613BC08756f51f46E841409E61C32f576F2f]
    │   │   ├─ [539] Storage::getBool(0x7ef800e7ca09c0c1063313b56290c06f6bc4bae0e9b7af3899bb7d5ade0403c8) [staticcall]
    │   │   │   └─ ← false
    │   │   └─ ← RialtoSimulator: [0x98D1613BC08756f51f46E841409E61C32f576F2f], false
    │   └─ ← "Division or modulo by 0"
    └─ ← "Division or modulo by 0"

Test result: FAILED. 0 passed; 1 failed; finished in 11.64ms

Failing tests:
Encountered 1 failing test in test/unit/RewardsPool.t.sol:RewardsPoolTest
[FAIL. Reason: Division or modulo by 0] testStartRewardsCycle_POC() (gas: 332890)

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project handle the case when the number of enabled multisig is 0 gracefully to not block the startRewardCycle transaction.

Lines of code

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

Vulnerability details

Impact

The validation rewards can be inaccuartedly displayed to user and the slahsed amount can be wrong when slashing happens.

Proof of Concept

note the function below:

	/// @notice Given a duration and an AVAX amt, calculate how much AVAX should be earned via validation rewards
	/// @param duration The length of validation in seconds
	/// @param avaxAmt The amount of AVAX the node staked for their validation period
	/// @return The approximate rewards the node should recieve from Avalanche for beign a validator
	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate();
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
	}

As outlined in the comment section, the function is intended to calculate how much AVAX should be earned via validation rewards

Besides display the reward, this function is also used in the function slash.

/// @notice Slashes the GPP of the minipool with the given index
/// @dev Extracted this because of "stack too deep" errors.
/// @param index Index of the minipool
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);
}

note the code:

uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

the slashedGGPAmt is calculated based on the AVAX reward amount.

However, the estimation of the validation rewards is not accurate.

According to the doc:

https://docs.avax.network/nodes/build/set-up-an-avalanche-node-with-microsoft-azure

Running a validator and staking with Avalanche provides extremely competitive rewards of between 9.69% and 11.54% depending on the length you stake for.

This implies that the staking length affect staking rewards, but this is kind of vague. What is the exact implementation of the reward calculation?

The implementation is linked below:

https://github.com/ava-labs/avalanchego/blob/master/vms/platformvm/reward/calculator.go#L40

// Reward returns the amount of tokens to reward the staker with.
//
// RemainingSupply = SupplyCap - ExistingSupply
// PortionOfExistingSupply = StakedAmount / ExistingSupply
// PortionOfStakingDuration = StakingDuration / MaximumStakingDuration
// MintingRate = MinMintingRate + MaxSubMinMintingRate * PortionOfStakingDuration
// Reward = RemainingSupply * PortionOfExistingSupply * MintingRate * PortionOfStakingDuration
func (c *calculator) Calculate(stakedDuration time.Duration, stakedAmount, currentSupply uint64) uint64 {
	bigStakedDuration := new(big.Int).SetUint64(uint64(stakedDuration))
	bigStakedAmount := new(big.Int).SetUint64(stakedAmount)
	bigCurrentSupply := new(big.Int).SetUint64(currentSupply)

	adjustedConsumptionRateNumerator := new(big.Int).Mul(c.maxSubMinConsumptionRate, bigStakedDuration)
	adjustedMinConsumptionRateNumerator := new(big.Int).Mul(c.minConsumptionRate, c.mintingPeriod)
	adjustedConsumptionRateNumerator.Add(adjustedConsumptionRateNumerator, adjustedMinConsumptionRateNumerator)
	adjustedConsumptionRateDenominator := new(big.Int).Mul(c.mintingPeriod, consumptionRateDenominator)

	remainingSupply := c.supplyCap - currentSupply
	reward := new(big.Int).SetUint64(remainingSupply)
	reward.Mul(reward, adjustedConsumptionRateNumerator)
	reward.Mul(reward, bigStakedAmount)
	reward.Mul(reward, bigStakedDuration)
	reward.Div(reward, adjustedConsumptionRateDenominator)
	reward.Div(reward, bigCurrentSupply)
	reward.Div(reward, c.mintingPeriod)

	if !reward.IsUint64() {
		return remainingSupply
	}

	finalReward := reward.Uint64()
	if finalReward > remainingSupply {
		return remainingSupply
	}

	return finalReward
}

note the reward calculation formula:

// Reward returns the amount of tokens to reward the staker with.
//
// RemainingSupply = SupplyCap - ExistingSupply
// PortionOfExistingSupply = StakedAmount / ExistingSupply
// PortionOfStakingDuration = StakingDuration / MaximumStakingDuration
// MintingRate = MinMintingRate + MaxSubMinMintingRate * PortionOfStakingDuration
// Reward = RemainingSupply * PortionOfExistingSupply * MintingRate * PortionOfStakingDuration

However, in the current ExpectedRewardAVA, the implementation is just:

AVAX reward rate * avax amount * duration / 365 days.

ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate();
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;

Clearly, the implementation of the avalanche side is more sophicated and accurate than the implemented ExpectedRewardAVA.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project make the ExpectedRewardAVA implementation match the implement

https://github.com/ava-labs/avalanchego/blob/master/vms/platformvm/reward/calculator.go#L40