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

Missing Input Validation in LSTRewardsSplitter.addFee Function

ahamedaathal

Relevant GitHub Links

https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L139-L144

Summary

The addFee function in LSTRewardsSplitter.sol allows the contract owner to add new fees without thorough validation of the _feeBasisPoints parameter beyond checking the total fees do not exceed 10000 basis points. This oversight can lead to scenarios where individual fees are set to excessively high values, inadvertently redirecting more rewards than intended or violating the system's economic model.

Vulnerability Details

contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol

function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}

Insufficient Validation of Individual Fees: The function only checks that the total fees do not exceed 10000 basis points (100%), but it does not validate that each individual fee is within a reasonable range. This lack of per-fee validation can allow the owner to assign an excessively high fee to a single receiver, potentially disrupting the intended reward distribution.

Impact

Allowing excessively high individual fees can lead to: Economic Imbalance: A single fee receiver could capture a disproportionate share of the rewards, undermining the system's fairness and economic incentives. Unexpected Behavior: High fees might cause unintended behavior in downstream systems or integrations relying on predictable fee structures. Resource Drain: If fees are set too high, it could lead to rapid depletion of rewards allocated for legitimate purposes.

PoC

test case using Hardhat and ethers.js that demonstrates the risk of missing input validation in the addFee function:

import { expect } from 'chai';
import { ethers } from 'hardhat';
import { Contract } from 'ethers';
​
describe('LSTRewardsSplitter Input Validation Test', function () {
let splitter: Contract;
let owner: any;
let nonOwner: any;
​
beforeEach(async function () {
[owner, nonOwner] = await ethers.getSigners();
​
// Deploy the LSTRewardsSplitter
const Splitter = await ethers.getContractFactory('LSTRewardsSplitter');
splitter = await Splitter.deploy(ethers.constants.AddressZero, [], owner.address);
await splitter.deployed();
});
​
it('Should allow adding a fee that keeps total fees within limit', async function () {
await expect(
splitter.connect(owner).addFee(nonOwner.address, 500)
).to.not.be.reverted;
​
const fees = await splitter.getFees();
expect(fees.length).to.equal(1);
expect(fees[0].receiver).to.equal(nonOwner.address);
expect(fees[0].basisPoints).to.equal(500);
});
​
it('Should revert when adding a fee that exceeds total fees', async function () {
await splitter.connect(owner).addFee(nonOwner.address, 10000);
await expect(
splitter.connect(owner).addFee(nonOwner.address, 1)
).to.be.revertedWith("FeesExceedLimit");
});
​
it('Should allow adding multiple fees with cumulative limit', async function () {
await splitter.connect(owner).addFee(owner.address, 3000);
await splitter.connect(owner).addFee(nonOwner.address, 4000);
await splitter.connect(owner).addFee(owner.address, 3000); // Total = 10000
​
const fees = await splitter.getFees();
expect(fees.length).to.equal(3);
});
​
it('Should not restrict individual fee amounts as long as total does not exceed limit', async function () {
// Adding a single fee with 10000 basis points
await expect(
splitter.connect(owner).addFee(nonOwner.address, 10000)
).to.not.be.reverted;
​
const fees = await splitter.getFees();
expect(fees.length).to.equal(1);
expect(fees[0].basisPoints).to.equal(10000);
});
});

Deploy the LSTRewardsSplitter contract with an initial empty fees array.

Test Cases: - Valid Fee Addition: Verify that adding a fee that does not cause the total to exceed 10000 basis points succeeds. -

Exceeding Total Fees: Attempt to add a fee that would push the total fees over 10000 basis points, expecting the transaction to revert.

- Multiple Fees Within Limit: Add multiple fees whose cumulative basis points do not exceed 10000, ensuring successful addition.

- Single Maximum Fee: Add a single fee of 10000 basis points to verify that the contract allows fees equal to the maximum allowed.

Expected Outcomes: - The contract should allow fees to be added as long as the total basis points do not exceed 10000. - The contract should reject any addition that causes the total to surpass the 100% threshold.

Tools Used

Manual Code Review

Recommendations

Implement Per-Fee Validation: - Introduce checks to ensure that each individual fee does not exceed a reasonable limit (e.g., not more than 10% per fee) to prevent disproportionate allocations.

function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
require(_receiver != address(0), "Invalid receiver");
require(_feeBasisPoints > 0, "Fee must be positive");
require(_feeBasisPoints <= 500, "Individual fee exceeds maximum allowed"); // Example: Max 5%
​
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}

Enforce Minimum and Maximum Fee Sizes: - Define acceptable ranges for fees to maintain system stability and fairness.

uint256 public constant MAX_INDIVIDUAL_FEE_BP = 500; // 5%
uint256 public constant MIN_INDIVIDUAL_FEE_BP = 10; // 0.1%
​
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
require(_receiver != address(0), "Invalid receiver");
require(_feeBasisPoints >= MIN_INDIVIDUAL_FEE_BP, "Fee below minimum");
require(_feeBasisPoints <= MAX_INDIVIDUAL_FEE_BP, "Fee exceeds maximum");
​
fees.push(Fee(_receiver, _feeBasisPoints));
require(_totalFeesBasisPoints() <= 10000, "Fees exceed limit");
}

Emit Detailed Events: - Emit events detailing fee additions, including the fee amount and receiver, to facilitate transparency and off-chain monitoring.

event FeeAdded(address indexed receiver, uint256 basisPoints);
​
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
// Existing validations...
fees.push(Fee(_receiver, _feeBasisPoints));
emit FeeAdded(_receiver, _feeBasisPoints);
// Existing total fee check...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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