Submission Details
Severity:
The way creating double arrays for receivers/feeAmounts in function _updateStrategyRewards with no checking _strategyIdxs in stakingPool contract leads to unexpected results
Summary
Creating double arrays receivers/feeAmounts in function _updateStrategyRewards by using ```strategies.length +1 ```, the inconsistency between _strategyIdxs and the receivers' length, which can lead to some strategies's receivers can't receive the expected rewards
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
int256 totalRewards;
uint256 totalFeeAmounts;
uint256 totalFeeCount;
// root reason: creating receivers by specifying the length as strategies.length + 1
address[][] memory receivers = new address[][]();
uint256[][] memory feeAmounts = new uint256[][]();
Vulnerability Details
The scenarios:
Three users had staked.
As staking-pool.test.ts shows: there are three Strategies, and Strategy1 and Strategy2 receive 110*10^18 link tokens as rewards
Strategy1 and Strategy2 all have one receiver that should receive the corresponding rewards.
stakingPool contract has two receivers who will receive the corresponding rewards.
Below supply two POCs; the first is the normal test, which shows each receiver should receive how much lst balance, and the other shows one receiver doesn't receive the expected rewards(lst token) instead of them remaining in the the stakingPool contract.
The core problem is when creating double arrays receivers/feeAmounts by using ```strategies.length + 1``` and the function doesn't check _strategyIdxs. so the owner can input more strategyIdxs such as [0,0,0,1] (current receivers. length=3), which leads to the last strategyIndex(1) will be covered by the stakingPool's receiver's logic.
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
int256 totalRewards;
uint256 totalFeeAmounts;
uint256 totalFeeCount;
address[][] memory receivers = new address[][]();
uint256[][] memory feeAmounts = new uint256[][]();
..................
// calulate fees if net positive rewards were earned
// if the _strategyIdxs length is strategies.length + 1, such as [0,0,0,1]
// the strategy1 receiver related info will be covered by below logic.
// meanwhile strategy1's totalDeposits was updated
if (totalRewards > 0) {
receivers[receivers.length - 1] = new address[]();
feeAmounts[feeAmounts.length - 1] = new uint256[]();
totalFeeCount += fees.length;
for (uint256 i = 0; i < fees.length; i++) {
receivers[receivers.length - 1][i] = fees[i].receiver;
feeAmounts[feeAmounts.length - 1][i] =
(uint256(totalRewards) * fees[i].basisPoints) /
10000;
totalFeeAmounts += feeAmounts[feeAmounts.length - 1][i];
}
}
Normal POC works as expected.
// Below POC was added in test/core/staking-pool.test.ts
it('updateStrategies Rewards work as expected', async () => {
const { accounts, adrs, stakingPool, token, strategy1, strategy2, erc677Receiver, stake } =
await loadFixture(deployFixture)
// As strategy using StrategyMock, for tesgt, just change the strategy2 owner as accounts[5], which also as the receiver for the strategy1.
// To make the StrategyMock's related receiver cann work, change the feeBasisPoints = 1000 https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/test/StrategyMock.sol#L35
await strategy2.transferOwnership(accounts[5])
assert.equal(await strategy2.owner(), accounts[5])
assert.equal(await strategy1.owner(), accounts[0])
//each strategy capacity: strategy1 capacity: 1000 || strategy2 capacity: 2000 || strategy3 capacity: 2000
// acount[1]/acount[2]/acount[3] deposit link to stakingPool
await stake(1, 2000)
await stake(2, 1000)
await stake(3, 2000)
// after depsot: strategy1 1000 || strategy2 2000 || strategy3 2000
console.log('Three users stake link token to stakingPool, to confirm staking work well')
console.log('stakingPool.totalStaked():', await stakingPool.totalStaked())
console.log('stakingPool.totalShares():', await stakingPool.totalShares())
console.log('strategy1 and strategy2 reveive 1100*10^18 link token as rewards')
await token.transfer(adrs.strategy1, toEther(1100))
await token.transfer(adrs.strategy2, toEther(1100))
let strategyRewards = await stakingPool.getStrategyRewards([0, 1])
console.log('strategyRewards:', strategyRewards)
// (totalRewards, totalFees)
console.log(
'strategyRewards: totalRewards',
strategyRewards[0],
'totalFees:',
strategyRewards[1]
)
await stakingPool.updateStrategyRewards([0, 1], '0x')
console.log('after updateStrategyRewards([0,1]')
console.log('stakingPool.totalStaked():', await stakingPool.totalStaked())
console.log('stakingPool.totalShares():', await stakingPool.totalShares())
strategyRewards = await stakingPool.getStrategyRewards([0, 1])
console.log('strategyRewards 2:', strategyRewards)
console.log(
'strategyRewards: totalRewards',
strategyRewards[0],
'totalFees:',
strategyRewards[1]
)
let stakingPoolReceivers = new Array()
const stakingPoolFees = await stakingPool.getFees()
for (var i = 0; i < stakingPoolFees.length; i++) {
stakingPoolReceivers[i] = stakingPoolFees[i].receiver
console.log(
'stakingpool fee recevier:',
stakingPoolReceivers[i],
'stakingPool.sharesOf(receiver):',
await stakingPool.sharesOf(stakingPoolReceivers[i])
)
}
// show strategy1 and strategy2's receiver's lst balance
console.log('strategy1 receiver lst balance:', await stakingPool.sharesOf(accounts[0]))
console.log('strategy2 receiver lst balance:', await stakingPool.sharesOf(accounts[5]))
// the left share in zeroAddress(dead share) + stakingPool itselft
console.log('zero adrerss lst balance', await stakingPool.sharesOf(ethers.ZeroAddress))
console.log('stakingPool itself lst balance', await stakingPool.sharesOf(stakingPool.target))
assert.equal(
// accounts[1] accounts[2] accounts[3] users
// accounts[0] accounts[5] strategy1 and strategy2's receiver
// stakingPool receivers stakingPoolReceivers
// ethers.ZeroAddress dead share and stakingPool itself 1 lst
(await stakingPool.sharesOf(accounts[1])) +
(await stakingPool.sharesOf(accounts[2])) +
(await stakingPool.sharesOf(accounts[3])) +
(await stakingPool.sharesOf(accounts[0])) +
(await stakingPool.sharesOf(accounts[5])) +
(await stakingPool.sharesOf(stakingPoolReceivers[0])) +
(await stakingPool.sharesOf(stakingPoolReceivers[1])) +
(await stakingPool.sharesOf(ethers.ZeroAddress)) +
(await stakingPool.sharesOf(stakingPool.target)),
await stakingPool.totalShares()
)
})
POC for Unexpected results
Comparing the above result, strategy 2's receiver doesn't receive the related rewards;
The corresponding rewards remain in the staking pool contract.
And there are no recording that shows the reward balance should belong to whom
it("updateStrategies Rewards don't work as expected", async () => {
const { accounts, adrs, stakingPool, token, strategy1, strategy2, erc677Receiver, stake } =
await loadFixture(deployFixture)
// As strategy using StrategyMock, for test, just change the strategy2 owner as accounts[5], which also as the receiver for strategy2.
// To make the StrategyMock's related receiver can work, change the feeBasisPoints = 1000 https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/test/StrategyMock.sol#L35
await strategy2.transferOwnership(accounts[5])
assert.equal(await strategy2.owner(), accounts[5])
assert.equal(await strategy1.owner(), accounts[0])
//each strategy capacity: strategy1 capacity: 1000 || strategy2 capacity: 2000 || strategy3 capacity: 2000
// acount[1]/acount[1]/acount[1] deposit link to stakingPool
await stake(1, 2000)
await stake(2, 1000)
await stake(3, 2000)
// after depsot: strategy1 1000 || strategy2 2000 || strategy3 2000
console.log('Three users stake link token to stakingPool, to make sure staking work well')
console.log('stakingPool.totalStaked():', await stakingPool.totalStaked())
console.log('stakingPool.totalShares():', await stakingPool.totalShares())
console.log('strategy1 and strategy2 reveive 1100*10^18 link token as rewards')
await token.transfer(adrs.strategy1, toEther(1100))
await token.transfer(adrs.strategy2, toEther(1100))
let strategyRewards = await stakingPool.getStrategyRewards([0, 1])
console.log('strategyRewards:', strategyRewards)
// totalRewards, totalFees)
console.log(
'strategyRewards: totalRewards',
strategyRewards[0],
'totalFees:',
strategyRewards[1]
)
console.log('owner updateStrategyRewards without checking the _strategyIdxs thoroughly')
// when the _strategyIdxs's lengh equal Strategies length+1 (current 4), the code don't validate the logic, and can continue executing
await stakingPool.updateStrategyRewards([0, 0, 0, 1], '0x')
console.log('after updateStrategyRewards([0, 0, 0, 1]')
console.log('stakingPool.totalStaked():', await stakingPool.totalStaked())
console.log('stakingPool.totalShares():', await stakingPool.totalShares())
// although seems that had been updated strategy1's rewards, and below show the strategy1's rewards have been distributed, but for the receiver doesn't receive the rewards
strategyRewards = await stakingPool.getStrategyRewards([1])
console.log('strategyRewards 2:', strategyRewards)
// totalRewards, totalFees)
console.log(
'strategyRewards: totalRewards',
strategyRewards[0],
'totalFees:',
strategyRewards[1]
)
// As strategy2 apply StrategyMock, For strategy2, receiver is accounts[5]
// ****** the unexpected result the receiver doesn't receive the related lst *****
console.log('strategy2 receiver lst balance:', await stakingPool.sharesOf(accounts[5]))
let stakingPoolReceivers = new Array()
const stakingPoolFees = await stakingPool.getFees()
for (var i = 0; i < stakingPoolFees.length; i++) {
stakingPoolReceivers[i] = stakingPoolFees[i].receiver
console.log(
'stakingpool fee recevier:',
stakingPoolReceivers[i],
'stakingPool.sharesOf(receiver):',
await stakingPool.sharesOf(stakingPoolReceivers[i])
)
}
// For strategy1, receiver is accounts[0]
console.log('strategy1 receiver lst balance:', await stakingPool.sharesOf(accounts[0]))
console.log('strategy2 receiver lst balance:', await stakingPool.sharesOf(accounts[5]))
// the left share in zeroAddress(dead share) + stakingPool itselft
console.log('zero adrerss lst balance', await stakingPool.sharesOf(ethers.ZeroAddress))
// the lst balance should be sent to strategy1's receiver, but still remains in the stakingPool contract
// the critical problem is there is no recording that the reamaining lst balance should belong to strategy1's receiver
console.log('stakingPool itself lst balance', await stakingPool.sharesOf(stakingPool.target))
assert.equal(
// accounts[1] accounts[2] accounts[3] users
// accounts[0] accounts[5] strategy1 and strategy2's receiver
// stakingPool receivers stakingPoolReceivers
// ethers.ZeroAddress dead share and stakingPool itself 1 lst
(await stakingPool.sharesOf(accounts[1])) +
(await stakingPool.sharesOf(accounts[2])) +
(await stakingPool.sharesOf(accounts[3])) +
(await stakingPool.sharesOf(accounts[0])) +
(await stakingPool.sharesOf(accounts[5])) +
(await stakingPool.sharesOf(stakingPoolReceivers[0])) +
(await stakingPool.sharesOf(stakingPoolReceivers[1])) +
(await stakingPool.sharesOf(ethers.ZeroAddress)) +
(await stakingPool.sharesOf(stakingPool.target)),
await stakingPool.totalShares()
)
})
Impact
Related owner executing updateStrategyRewards without carefully checking leads to the situation of some strategies' receivers losing their rewards.
Although the rewards remain in the soakingPool, there is no recording these rewards should belong to whom. So, no matter whether the CommunityVCS or the OperatorVCS can't get the expected rewards as the stake.link shows.
Although it can be conducted from scratch, analyzing the problem to calculate the expected rewards for the related strategy, with ignorance errors accumulating, it will become increasingly difficult to figure out the right expected rewards for the related strategies.
Tools Used
Mannal and hardhat
Recommendations
Add necessary validation for _updateStrategyRewards. One solution is that can't input the same strategy Id.
// if _strategyIdxs have same strategyId, revert
modifier checkStrategyIdxs(uint256[] memory _strategyIdxs) {
// this can be moved to updateStrategyRewards function
require(_strategyIdxs.length > 0, "Empty strategyId");
uint256 compareStrategyId = _strategyIdxs[0];
for (uint256 i = 1; i < _strategyIdxs.length; i++) {
require(compareStrategyId != _strategyIdxs[i], "inculde the same StrategyId");
compareStrategyId = _strategyIdxs[i];
}
_;
}
function _updateStrategyRewards(
uint256[] memory _strategyIdxs,
bytes memory _data
) private checkStrategyIdxs(_strategyIdxs) {
....
}
Updates
Prize pool breakdown
Total prize
50,000 USDC
nSLOC
2,53020 USDC / LOC
42,000 USDC
4,250 USDC
3,750 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.