The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Risk of reward loss due to deletions in the holders array, impacting liquidity stake-holders

JrNet

Summary

In the LiquidityPool contract, when a liquidity holder withdraws their entire staked amount from the pool, the corresponding position is deleted from the positions mapping, and the holder is removed from the holders array. However, a flaw in the logic exists: positions that are still pendingStake processing after this withdrawal action (entire stake removal) are not considered for reward distribution. This oversight results in potential loss of rewards for affected positions.

empty(): Checks for stake in the existing positions of the caller.

// File: LiquidationPool.sol
function empty(Position memory _position) private pure returns (bool) {
return _position.TST == 0 && _position.EUROs == 0;
}

deleteHolder(): deletes the holder from holders array.

// File: LiquidityPool.sol
function deleteHolder(address _holder) private {
...
holders.pop();
}

consolidatePendingStakes(): Never checks for holder existence in holders array. Which is essential to spread and claim reward tokens.

// File: LiquidityPool.sol
function consolidatePendingStakes() private {
...
positions[_stake.holder].holder = _stake.holder;
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));
...
}

Vulnerability Details

  1. Scenario:

    • User 1 opens a new position with 10 TST.

    • The position is processed the next day.

  2. Scenario:

    • On the second day, User 1 opens several new positions by calling increasePosition, and they are sent to pendingStakes.

    • Assuming User 1 took another position with 10 TST.

    • User 1 wants to decrease the position by the amount of 10 TST (available to redeem as processed from pending to positions).

  3. Outcome:

    • User 1 assumes that they held a position at LP and earned rewards.

    • But on the decrease position call, User 1 deleted their address from holders.

    • To receive and claim rewards, the code holds the logic to iterate through the holders array.

// File: LiquidityPool.sol
function distributeFees(uint256 _amount) external onlyManager {
...
/*
Iterates through the list of holders and calculates the portion of fees each holder
is entitled to based on their TST holdings. The result is added to their EUROs balance
in their respective positions.
*/
for (uint256 i = 0; i < holders.length; i++) {
// @audit User-1 never existed
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
...
}

  1. Result:

    • User 1 never acquired rewards for this holding.

Hardhat Test PoC

Add the below test code to the liquidation pool test

// File: test/liquidationPool.js
describe('bad position', async () => {
it('delete holder with existing position', async () => {
const tstStake = ethers.utils.parseEther('20');
await TST.mint(user1.address, tstStake);
await TST.connect(user1).approve(LiquidationPool.address, tstStake);
​
const slice = ethers.utils.parseEther('10');
​
/*
*******************************************************************
* User 1 opens a position in LP with 10 TST *
* *****************************************************************
*/
await LiquidationPool.connect(user1).increasePosition(slice, 0);
​
// user 1 stakes 10 EUROs
let { _position } = await LiquidationPool.position(user1.address);
expect(_position.TST).to.equal(slice);
​
// pass a day
await fastForward(DAY);
​
/*
*******************************************************************
* User 1 increase position in LP with another 10 TST *
* *****************************************************************
*/
await LiquidationPool.connect(user1).increasePosition(slice, 0);
​
/*
******************************************************************************
* User 1 decrease position with another 10 TST *
* As the position is empty it removes user 1 from holders array *
* ****************************************************************************
*/
// decrease position remove all stake
await LiquidationPool.connect(user1).decreasePosition(slice, 0);
​
// pass a day
await fastForward(DAY);
​
/*
*******************************************************************
* User 2 opens position with 100_000 TST *
* Rewards are to be distributed by this call *
* *****************************************************************
*/
​
// assigning fees to LPManager
const fees = ethers.utils.parseEther('100');
await EUROs.mint(LiquidationPoolManager.address, fees);
​
tstStakeValue = ethers.utils.parseEther('100000');
await TST.mint(user2.address, tstStakeValue);
await TST.connect(user2).approve(LiquidationPool.address, tstStakeValue);
await LiquidationPool.connect(user2).increasePosition(tstStakeValue, 0);
​
/*
*******************************************************************************
* Does staker receive rewards? - NO *
* Until user 1 decides to increase position he receives no rewards *
* *****************************************************************************
*/
({ _position } = await LiquidationPool.position(user1.address));
// staked 10 TST tokens but received 0 EUROs as rewards
expect(_position.EUROs).to.equal(ethers.utils.parseEther('0'));
console.log(_position);
​
/*
*******************************************************************
* Are Staked tokens safe? - YES *
* *****************************************************************
*/
await LiquidationPool.connect(user1).decreasePosition(slice, 0);
({ _position } = await LiquidationPool.position(user1.address));
expect(_position.TST).to.equal(0);
expect(await TST.balanceOf(user1.address)).to.equal(tstStake);
});
});

Sample Result:

LiquidationPool
bad position
[
'0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
BigNumber { value: "10000000000000000000" },
BigNumber { value: "0" },
holder: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
TST: BigNumber { value: "10000000000000000000" },
EUROs: BigNumber { value: "0" } @> // no rewards earned on stake
]
✔ delete holder with existing position (555ms)

Impact

This is an edge case where a stake-holder assumes to be receiving rewards on their stake but doesn't. Stake-holders fail to receive rewards until they call the increasePosition function which is as rare action as the cause to vulnerability.

Tools Used

Hardhat

Recommendations

If holder holds pending stakes return false.

// File: LiquidationPool.sol
function deleteHolder(address _holder) private {
+ // Check if there are pending stakes for the holder
+ (uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
+ if (_pendingTST > 0 || _pendingEUROs > 0) {
+ // If there are pending stakes, do not delete the holder
+ return;
+ }
+
for (uint256 i = 0; i < holders.length; i++) {
if (holders[i] == _holder) {
holders[i] = holders[holders.length - 1];
holders.pop();
}
}
}
​
Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

deletePosition-issye

Support

FAQs

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