Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: medium
Invalid

Principal Accounting Issues

Summary

The contract's logic for handling deposits and rewards can result in miscalculations of principalDeposits, particularly when new rewards are negative or insufficient. This mismanagement can lead to incorrect distribution of rewards or even allow unauthorized withdrawals, compromising the integrity of the contract's reward splitting mechanism.

Vulnerability Details

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L101

function performUpkeep(bytes calldata) external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards);
} else if (uint256(newRewards) < controller.rewardThreshold()) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L116C14-L116C28

function splitRewards() external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards);
} else if (newRewards == 0) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}

The function performUpkeep() and splitRewards() rely on the difference between the contract's current token balance and principalDeposits to determine new rewards. However, there are scenarios where this calculation can fail.

When new rewards are negative, i.e., when more tokens are withdrawn or consumed than deposited, the contract tries to adjust principalDeposits by subtracting the negative value, which can lead to underflows.

This could happen if tokens are removed from the contract (through a method outside the scope of this contract) before upkeep is called, causing lst.balanceOf(address(this)) to be less than principalDeposits.

Additionally, when rewards are insufficient, i.e., less than the controller-defined threshold, the contract doesn’t properly handle the state, leading to possible inaccurate splits or halts in reward distribution.

Impact

Incorrect Principal Accounting: In the case of a negative reward scenario, principalDeposits is reduced by the absolute value of negative rewards. This can cause the stored principalDeposits to become inaccurate, leading to incorrect rewards being distributed in future upkeep or manual reward splitting operations.

Underflow and Withdrawal Risks: If principalDeposits is undercounted, the contract could allow withdrawals of more tokens than are actually deposited, resulting in a depletion of tokens from the contract.

Inconsistent Reward Splitting: The incorrect handling of negative rewards can cause the reward splitting mechanism to function incorrectly, which may lead to zero or insufficient rewards being split across fee receivers, causing operational failures

Tools Used

Recommendations

Enforce Principal Deposit Boundaries: Introduce explicit checks to ensure that principalDeposits cannot go negative under any circumstances. This can be done using simple require() statements to validate that principalDeposits remain positive after adjustments.

Handle Insufficient Rewards Gracefully: When new rewards are negative or insufficient, revert the transaction or handle the rewards more explicitly, preventing silent underflows. This will maintain the integrity of the accounting.

Track Deposits and Rewards Separately: Consider separating the principal deposits and rewards explicitly, ensuring that the logic differentiates between what’s deposited by the user and what is accrued through rewards.

Proof of Concept

To prove and explain the vulnerability related to Principal Accounting Issues, let's look deeper into the critical sections of the code in performUpkeep() and splitRewards() where principalDeposits is manipulated.

Vulnerable Code

function performUpkeep(bytes calldata) external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards); // Vulnerability here
} else if (uint256(newRewards) < controller.rewardThreshold()) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}
function splitRewards() external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards); // Vulnerability here
} else if (newRewards == 0) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}

Explanation of Vulnerability

In the lines:

if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards);
}

The problem lies in how the contract handles negative rewards:

Negative Reward Calculation: If lst.balanceOf(address(this)) (the token balance) is less than principalDeposits, the newRewards will be negative. The contract attempts to compensate for this by subtracting the negative reward from principalDeposits.

Underflow Risk: If principalDeposits is already small, subtracting this negative amount could cause an underflow. While Solidity 0.8+ has built-in underflow protection (reverting the transaction), the logic itself is flawed because it fails to handle such cases appropriately. The contract should not be adjusting principalDeposits for negative rewards at all.

Logical Error: Even if underflow is avoided, adjusting principalDeposits in this way can cause it to be incorrectly updated, affecting future deposit and reward calculations. This can lead to incorrect token withdrawals or reward splits.

Demonstration of the Issue

  1. Initial State:

    • principalDeposits = 1000 (tokens deposited into the contract).

    • lst.balanceOf(address(this)) = 500 (tokens in the contract balance due to some external action, e.g., a manual transfer out).

      When performUpkeep() is called:

    • newRewards = 500 - 1000 = -500.

    • The contract executes: principalDeposits -= uint256(-1 * newRewards), which is principalDeposits -= 500.

    • Result: principalDeposits = 1000 - 500 = 500, even though only 500 tokens are in the contract, leading to misalignment between actual balance and principalDeposits.

  2. Subsequent Withdrawals:

    • If the controller attempts to withdraw 1000 tokens (thinking that 1000 were deposited), it will overdraw from the contract since the actual balance is only 500.

Corrected Code (Recommended Fix)

Here’s a corrected version of the performUpkeep() and splitRewards() functions that properly handles negative rewards:

function performUpkeep(bytes calldata) external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
// Revert or handle negative rewards without adjusting principalDeposits
revert InsufficientRewards();
} else if (uint256(newRewards) < controller.rewardThreshold()) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}
function splitRewards() external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
// Do not adjust principalDeposits if rewards are negative
revert InsufficientRewards();
} else if (newRewards == 0) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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