Lines of code

Vulnerability details

Impact

node operators ggp rewards are distributed by function calculateAndDistributeRewards() which is called by Multisig and function calculateAndDistributeRewards() can only distribute current cycle rewards. the rewards are calculated based on user’s avaxAssignedHighWater and code would reset the value of the avaxAssignedHighWater so it can be calculated from zero for the next cycle. but if for one cycle Multisig don’t call this function for one of the node runners the value of the avaxAssignedHighWater won’t get reset and its value would be wrong for the next cycle and in the next cycle user would receive wrong amount of rewards (more than what supposed to receive). the problem is that contract should keep track of avaxAssignedHighWater for users in each cycle separately and don’t rely on external call to reset the value of the avaxAssignedHighWater for users.

Proof of Concept

This is calculateAndDistributeRewards() code:

	function calculateAndDistributeRewards(address stakerAddr, uint256 totalEligibleGGPStaked) external onlyMultisig {
		Staking staking = Staking(getContractAddress("Staking"));
		staking.requireValidStaker(stakerAddr);

		RewardsPool rewardsPool = RewardsPool(getContractAddress("RewardsPool"));
		if (rewardsPool.getRewardsCycleCount() == 0) {
			revert RewardsCycleNotStarted();
		}

		if (staking.getLastRewardsCycleCompleted(stakerAddr) == rewardsPool.getRewardsCycleCount()) {
			revert RewardsAlreadyDistributedToStaker(stakerAddr);
		}
		staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());
		uint256 ggpEffectiveStaked = staking.getEffectiveGGPStaked(stakerAddr);
		uint256 percentage = ggpEffectiveStaked.divWadDown(totalEligibleGGPStaked);
		uint256 rewardsCycleTotal = getRewardsCycleTotal();
		uint256 rewardsAmt = percentage.mulWadDown(rewardsCycleTotal);
		if (rewardsAmt > rewardsCycleTotal) {
			revert InvalidAmount();
		}

		staking.resetAVAXAssignedHighWater(stakerAddr);
		staking.increaseGGPRewards(stakerAddr, rewardsAmt);

		// check if their rewards time should be reset
		uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
		if (minipoolCount == 0) {
			staking.setRewardsStartTime(stakerAddr, 0);
		}
	}

As you can see the reward is calculated based on avaxAssignedHighWater and then code reset the value of the avaxAssignedHighWater by calling staking.resetAVAXAssignedHighWater(stakerAddr). so if in one cycle Multisig don’t call this function in time for a user then the value of the avaxAssignedHighWater won’t get reset for the user and protocol would use its value for the next cycle. this can cause wrong reward distribution because in the next cycle that user can have different Minipool and different staking amount and the issue can cause user to receive more rewards (other users lose their rewards). the bug would happen if multisig don’t call this function in time and it doesn’t require bad action from Multisig or wrong parameters, just by not calling this function by any reason the bug can happen.

Tools Used

VIM

Recommended Mitigation Steps

calculate avaxAssignedHighWater for users in each cycle separately (use two dimensions map that includes cycle number and user address) and don’t rely on external actor to call and reset the value of the avaxAssignedHighWater for user.

The text was updated successfully, but these errors were encountered:

All reactions