Summary
The _distributeToGauges function is vulnerable to two issues:
It may run Out of Gas if processing too many gauges.
A single gauge revert can disrupt the reward distribution for all other gauges.
Vulnerability Details
A single revert would occur if the notifyRewards function for a single gauge gets reverted due to any of the following cases:
RewardCapExceeded.
InsufficientRewardBalance.
/contracts/core/governance/gauges/BaseGauge.sol:354
354: function notifyRewardAmount(uint256 amount) external override onlyController updateReward(address(0)) {
355: if (amount > periodState.emission) revert RewardCapExceeded();
356:
357: rewardRate = notifyReward(periodState, amount, periodState.emission, getPeriodDuration());
358: periodState.distributed += amount;
359:
360: uint256 balance = rewardToken.balanceOf(address(this));
361: if (rewardRate * getPeriodDuration() > balance) {
362: revert InsufficientRewardBalance();
363: }
364:
365: lastUpdateTime = block.timestamp;
366: emit RewardNotified(amount);
367: }
The 2nd revert could occur due to iteration over all the gauges twice.
/contracts/core/governance/gauges/GaugeController.sol:519
519: function distributeRevenue(
520: GaugeType gaugeType,
521: uint256 amount
522: ) external onlyRole(EMERGENCY_ADMIN) whenNotPaused {
523: if (amount == 0) revert InvalidAmount();
...
528: revenueShares[gaugeType] += veRAACShare;
529: _distributeToGauges(gaugeType, veRAACShare);
...
532: }
From the below code, it can be seen that:
In the first loop, we calculate the totalWeight and totalActiveGauges. Then, we iterate again over all gauges to distribute the rewards to the active ones.
/contracts/core/governance/gauges/GaugeController.sol:542
542: function _distributeToGauges(
543: GaugeType gaugeType,
544: uint256 amount
545: ) internal {
546: uint256 totalTypeWeight = 0;
547: uint256[] memory gaugeWeights = new uint256[](_gaugeList.length);
548: uint256 activeGaugeCount = 0;
549:
550:
551: for (uint256 i = 0; i < _gaugeList.length; i++) {
552: address gauge = _gaugeList[i];
553: if (gauges[gauge].isActive && gauges[gauge].gaugeType == gaugeType) {
554: gaugeWeights[i] = gauges[gauge].weight;
555: totalTypeWeight += gaugeWeights[i];
556: activeGaugeCount++;
557: }
558: }
559:
560: if (totalTypeWeight == 0 || activeGaugeCount == 0) return;
561:
562:
563: for (uint256 i = 0; i < _gaugeList.length; i++) {
564: address gauge = _gaugeList[i];
565: if (gauges[gauge].isActive && gauges[gauge].gaugeType == gaugeType) {
566: uint256 gaugeShare = (amount * gaugeWeights[i]) / totalTypeWeight;
567: if (gaugeShare > 0) {
568: IGauge(gauge).notifyRewardAmount(gaugeShare);
569: }
570: }
571: }
572: }
For one active gauge, the gasPrice is 148,882. However, with two gauges (one active and one non-active), the gasPrice increases to 154,968. This means looping over all gauges again in the second loop significantly increases the gas cost of the transaction. In this simple case, the difference is nearly 6,086 gas units.
If there are more than 193 active gauges, a DoS becomes permanent, as the gas cost for a single loop iteration is 154,968. The calculation is as follows:
POC :
apply this following git diff to GaugeController.test.js and run with command npx hardhat test
@@ -75,11 +75,11 @@ describe("GaugeController", () => {
await gaugeController.connect(gaugeAdmin).addGauge(
await raacGauge.getAddress(),
1, // RAAC type
- 0 // Initial weight
+ 10000 // Initial weight
);
// Initialize gauges
- await rwaGauge.grantRole(await rwaGauge.CONTROLLER_ROLE(), owner.address);
+ // await rwaGauge.grantRole(await rwaGauge.CONTROLLER_ROLE(), owner.address);
await raacGauge.grantRole(await raacGauge.CONTROLLER_ROLE(), owner.address);
});
@@ -87,8 +87,19 @@ describe("GaugeController", () => {
beforeEach(async () => {
await veRAACToken.mint(user1.address, ethers.parseEther("1000"));
await veRAACToken.mint(user2.address, ethers.parseEther("500"));
+ await rewardToken.mint(await raacGauge.getAddress() , ethers.parseEther("500"));// mint reward token to gauges
+ await rewardToken.mint(await rwaGauge.getAddress() , ethers.parseEther("500"));// mint reward token to gauges
+
});
+ it.only("test rewards distribution" , async function(){
+ let tx = await gaugeController.connect(emergencyAdmin).distributeRevenue(1 , ethers.parseEther("100"));
+ const receipt = await tx.wait();
+ console.log(`Gas Used (distributeRevenue): ${receipt.gasUsed.toString()}`);
+ const weeklyState = await raacGauge.periodState();
+ expect(weeklyState.distributed).to.equal(ethers.parseEther("80"));
+ })
+
Impact
The _distributeToGauges function can continuously fail, preventing the protocol from distributing the rewards and calculating the performanceShare. If the transaction fails, the corresponding gas cost will be lost, and the reward periods for all other gauges will not start. Although the distributeRewards function exists, it does not update the performanceShare for the protocol.
An attacker could front-run the distributeToGauges transaction with distributeRewards, causing it to revert if the distributed amount exceeds the allowed emission.
Tools Used
Manual Review
Recommendations
The following two points could help address the potential issues with the _distributeToGauges function:
-
Fix for Gauge Revert:
Wrap the notifyRewardAmount call in a try/catch block to ensure that a single revert does not prevent other gauges from receiving their reward amount.
-
Batch Processing and Iteration Optimization:
Call the function in batches and, for the second iteration, iterate only over the active gauges, rather than looping through the entire list of gauges again.