Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Incorrect Handling of Negative newRewards in LSTRewardsSplitter::checkUpkeep Results in State Inconsistencies and False Positives for checkUpkeep

heim

Summary

This report investigates a specific vulnerability within the checkUpkeep functions of the LSTRewardsSplitter contract. The primary issue revolves around handling reward calculations when new rewards are negative, which can cause incorrect upkeep triggers.

Vulnerability Details

The logic in the checkUpkeep function incorrectly handles cases where new rewards are negative. This leads the function to return true (indicating that upkeep is needed) even when it should not, resulting in unnecessary and potentially costly invocations of the performUpkeep function.

Code snippet:
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L85C3-L96C6

function checkUpkeep(bytes calldata) external view returns (bool, bytes memory) {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
​
if (newRewards < 0 || uint256(newRewards) >= controller.rewardThreshold()) {
return (true, bytes(""));
}
​
return (false, bytes(""));
}

POC

  • Copy this test into the existing test file test/core/lst-rewards-splitter.test.ts

it('checkUpkeep should work correctly2', async () => {
const { controller, token, splitter0, splitter1 } = await loadFixture(deployFixture)
​
// Case 1: No new rewards, should return false
await token.transfer(splitter0.target, toEther(90))
await token.transfer(splitter1.target, toEther(80))
assert.deepEqual(await controller.checkUpkeep('0x'), [
false,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[false, false]]),
])
​
// Case 2: New rewards less than reward threshold, should return false
await token.transfer(splitter1.target, toEther(20))
assert.deepEqual(await controller.checkUpkeep('0x'), [
true,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[false, true]]),
])
​
// Case 3: New rewards equal to reward threshold, should return true
await token.transfer(splitter0.target, toEther(10))
assert.deepEqual(await controller.checkUpkeep('0x'), [
true,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[true, true]]),
])
​
// Perform upkeep
await controller.performUpkeep(
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[true, true]])
)
assert.deepEqual(await controller.checkUpkeep('0x'), [
false,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[false, false]]),
])
​
// Case 4: Change reward threshold and check again
await token.setMultiplierBasisPoints(5000)
assert.deepEqual(await controller.checkUpkeep('0x'), [
true,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[true, true]]),
])
​
// Case 5: New rewards greater than reward threshold, should return true
await token.transfer(splitter0.target, toEther(100))
​
console.log('After transfer:')
console.log('splitter0 balance:', fromEther(await token.balanceOf(splitter0.target)))
console.log('splitter1 balance:', fromEther(await token.balanceOf(splitter1.target)))
console.log('principalDeposits:', fromEther(await splitter0.principalDeposits()))
console.log('controller balance:', fromEther(await token.balanceOf(controller.target)))
const checkUpkeepResult = await controller.checkUpkeep('0x')
console.log('checkUpkeep result:', checkUpkeepResult)
​
// Additional debugging logs
const controllerBalance = await token.balanceOf(controller.target)
const principalDeposits = await splitter0.principalDeposits()
const newRewards = controllerBalance - principalDeposits
const rewardThreshold = await controller.rewardThreshold()
console.log('controllerBalance:', fromEther(controllerBalance))
console.log('principalDeposits:', fromEther(principalDeposits))
console.log('newRewards:', fromEther(newRewards))
console.log('rewardThreshold:', fromEther(rewardThreshold))
​
// Juster forventede resultater basert på korrekt logikk
const expectedNewRewards = controllerBalance - principalDeposits
assert.deepEqual(checkUpkeepResult, [
expectedNewRewards >= rewardThreshold,
ethers.AbiCoder.defaultAbiCoder().encode(
['bool[]'],
[[expectedNewRewards >= rewardThreshold, expectedNewRewards >= rewardThreshold]]
),
])
​
// Change reward threshold and validate
await token.setMultiplierBasisPoints(20000)
assert.deepEqual(await controller.checkUpkeep('0x'), [
false,
ethers.AbiCoder.defaultAbiCoder().encode(['bool[]'], [[false, false]]),
])
})

  • Output:

After transfer:
splitter0 balance: 85
splitter1 balance: 20
principalDeposits: 70
controller balance: 0
checkUpkeep result: Result(2) [
true,
'0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001'
]
controllerBalance: 0
principalDeposits: 70
newRewards: -70
rewardThreshold: 100

The function checkUpkeep returned true incorrectly when newRewards was negative. The correct behavior should involve a more explicit check to avoid triggering upkeep without valid positive rewards.

Impact

Impact on Protocol: Medium.
There is some level of disruption, as unnecessary maintenance operations could lead to wasted gas and reduce the protocol's efficiency.

Likelihood of Exploitation: Medium
The likelihood of encountering this issue is moderate, as it depends on specific conditions where newRewards go negative.

Combining these factors, the severity of this finding is classified as Medium. It is crucial to address this to prevent unnecessary costs and inefficiencies.

Tools Used

  • Hardhat

Recommendations

Update checkUpkeep Logic: Explicitly handle negative newRewards values to avoid unnecessary upkeep calls.

Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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