The Standard

DeFiHardhat

20,000 USDC

View results

Previous Next

Submission Details

Severity: high

Valid

Stakers could be unintentionally excluded from receiving staking rewards

spacelord47

Summary

Due to a flaw in the "staking position management" logic, stakers, after updating their position, may end up in a situation when they are excluded from staking rewards.

Issue Context

One of the core features of the Protocol is a staking mechanism. Users incentivized to stake their TST and EUROs tokens to earn passive income. The treasury for rewards distribution is built of two sources:

Fee, in the form of EUROs tokens, collected from SmartVaultV3 users, when they mint/burn EUROs tokens
ERC20 tokens and ETH received from SmartVaultV3 (auto) liquidations.

Here is how the staking/rewarding process looks like for stakers:

User initiates position creation by calling LiquidationPool.increasePosition

134: function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {

135: require(_tstVal > 0 || _eurosVal > 0);

136: consolidatePendingStakes(); // @audit This function will advance all "mature" PendingStake to Position

137: ILiquidationPoolManager(manager).distributeFees();

138: if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);

139: if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);

140: pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));

141: addUniqueHolder(msg.sender);

142: }

In this function, LiquidationPool creates a PendingStake for user (line 140), which can be migrated to a "mature" Position after a delay (1d). A PendingStake will start collecting the first type of fees(EUROs) immediately, but ERC20 tokens only after migrating to a Position (this mechanism prevents users from abusing vaults liquidations).
LiquidationPool also adds user's address to the holders array (line 141). This array is used during rewards distribution.
When 1d "timeout" passed, any call to increasePosition, decreasePosition or distributeAssets will trigger consolidatePendingStakes function, and user's PendingStake will be migrated to Position:

119: function consolidatePendingStakes() private {

120: uint256 deadline = block.timestamp - 1 days;

121: for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {

122: PendingStake memory _stake = pendingStakes[uint256(i)];

123: if (_stake.createdAt < deadline) {

124: positions[_stake.holder].holder = _stake.holder;

125: positions[_stake.holder].TST += _stake.TST;

126: positions[_stake.holder].EUROs += _stake.EUROs;

127: deletePendingStake(uint256(i));

128: // pause iterating on loop because there has been a deletion. "next" item has same index

129: i--;

130: }

131: }

132: }

At this stage, user have "fully activated" position, and started collection both types of rewards.
At some point, user may decide to close position by calling decreasePosition function. And if user withdrawing all staked assets, the position will be closed by removing corresponding Position, and removing user's address from holders (line 161):

149: function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {

150: consolidatePendingStakes();

151: ILiquidationPoolManager(manager).distributeFees();

152: require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");

153: if (_tstVal > 0) {

154: IERC20(TST).safeTransfer(msg.sender, _tstVal);

155: positions[msg.sender].TST -= _tstVal;

156: }

157: if (_eurosVal > 0) {

158: IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);

159: positions[msg.sender].EUROs -= _eurosVal;

160: }

161: if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);

162: }

Issue Details

The issue in this "pending-mature position" mechanism is related to the fact that users could have both "pending"(PendingStake) and "mature"(Position) positions, so if a user fully closes his "mature" position, then he gets removed from the holders array. And when his "pending" position migrated to "mature", it will not start collecting rewards, because user is not present in holders, and only holders will be accounted during rewards distribution.

So, user's Position will exist, with user's funds locked, but it will not collect any rewards.

And here is how this scenario can be practically achieved:

User creates PendingSake
24h passed and PendingStake migrated to Position
Some time passed and User decides to increase his deposit, and creates a new PendingStake
24h not yet passed since the last PendingStake, User decides to withdraw his Position, and keep (current) PendingStake (the actual incentives for this decision is related to the specifics of user's strategies, e.g. rebalancing)
24h passed since the last PendigStake created, so it's migrated to Position
Expected state: User's position started collecting rewards
Actual state: User's position excluded from rewards, because user not present in holders

Impact

Affected users will miss both types of rewards (most importantly rewards from liquidations) for funds they deposited.

Note that, there also no related events emitted (e.g. position activated) in the current logic, so the user might stay unaware of his position "not yielding" for unknown amount of time.

PoC

The following test case implements the scenario described in the previous sections. It shows how a user can end up being excluded from receiving staking rewards.

Please add it to test/liquidationPool.js and run with npx hardhat test.

it.only('removing position while having a pending stake will result in loss of all future rewards', async () => {

const userFirstTSTStake = ethers.utils.parseEther('10');

const userEUROsStake = 0;

// Mint TST tokens for our user, so it can start staking

await TST.mint(user1.address, userFirstTSTStake);

// Approve pool for TST, so we can deposit assets for staking

await TST.connect(user1).approve(LiquidationPool.address, userFirstTSTStake);

// User initiates position creation by creating a pending stake

await LiquidationPool.connect(user1).increasePosition(userFirstTSTStake, userEUROsStake);

// Now our pool manager (artificially) receives some fees (in prod environment, it will come for SmartVault users)

const poolFeeTreasuryInEUROs = ethers.utils.parseEther("100");

await EUROs.mint(LiquidationPoolManager.address, poolFeeTreasuryInEUROs);

// Advance in time(1d), so user's pending stake can migrate to a new position and start collecting fees

await network.provider.send("evm_increaseTime", [86_400]);

// Now user decides to increase his position. During the call to "increasePosition" his initial pending stake

// will migrate to actual position and collect fees

await TST.mint(user1.address, userFirstTSTStake);

await TST.connect(user1).approve(LiquidationPool.address, userFirstTSTStake);

const userSecondTSTStake = ethers.utils.parseEther("5");

await LiquidationPool.connect(user1).increasePosition(userSecondTSTStake, userEUROsStake);

// Confirm that fee distributed properly: user receives 50% from 100 EUROs treasury to the position balance,

// remaining 50% transferred to the protocol

let { _position} = await LiquidationPool.position(user1.address);

expect(_position.holder).to.equal(user1.address);

expect(_position.TST).to.equal(userFirstTSTStake.add(userSecondTSTStake));

expect(_position.EUROs).to.equal(ethers.utils.parseEther("50"));

// Now user decides to remove his (active) position. But the (second) pending stake is still in the pool

const userCollectedFeesInEUROs = ethers.utils.parseEther("50");

await LiquidationPool.connect(user1).decreasePosition(userFirstTSTStake, userCollectedFeesInEUROs);

// Confirm that position removed.

// Note that the "position" function accounts for both - pending stakes and positions. So we now should receive

// values for our second "pending stake"

({ _position} = await LiquidationPool.position(user1.address));

expect(_position.TST).to.equal(userSecondTSTStake);

expect(_position.EUROs).to.equal("0");

// Advance in time again, so our second stake can migrate to position and start collecting fees

await network.provider.send("evm_increaseTime", [86_400]);

// Another portion of fees arrives to the pool's treasury

await EUROs.mint(LiquidationPoolManager.address, poolFeeTreasuryInEUROs);

// Now, as with first stake, our (second) position should be ready to collect fees.

// And let's trigger rewards distribution by introducing new user to the pool

await TST.mint(user2.address, userFirstTSTStake);

await TST.connect(user2).approve(LiquidationPool.address, userFirstTSTStake);

await LiquidationPool.connect(user2).increasePosition(userFirstTSTStake, 0);

// At this stage, user1 should receive 50 EUROs in rewards from the last distribution, but it will not happen.

({ _position} = await LiquidationPool.position(user1.address));

expect(_position.holder).to.equal(user1.address);

expect(_position.TST).to.equal(userSecondTSTStake);

// user1 receives "0" in rewards from the last distribution

expect(_position.EUROs).to.equal("0");

});

Note: the patch from the "Recommendation" section fixes the issue and may be used with the same test case to confirm that user should be eligible for rewards.

Recommendation

After reviewing all usages of the holders array in LiquidationPool, it seems like there is no need to update holders in the increasePosition function (which creates PendingStake), because holders is used only to access members of positions mapping.
So we can move logic which updates holders array to consolidatePendingStakes:

diff --git a/contracts/LiquidationPool.sol b/contracts/LiquidationPool.sol

--- a/contracts/LiquidationPool.sol (revision c12272f2eec533019f2d255ab690f6892027f112)

+++ b/contracts/LiquidationPool.sol (date 1704216444505)

@@ -124,6 +124,7 @@

positions[_stake.holder].holder = _stake.holder;

positions[_stake.holder].TST += _stake.TST;

positions[_stake.holder].EUROs += _stake.EUROs;

+ addUniqueHolder(_stake.holder);

deletePendingStake(uint256(i));

// pause iterating on loop because there has been a deletion. "next" item has same index

i--;

@@ -138,7 +139,6 @@

if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);

if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);

pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));

- addUniqueHolder(msg.sender);

}

function deletePosition(Position memory _position) private {

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago

Submission Judgement Published

Validated

Assigned finding tags:

pendingstake-dos

hrishibhat Lead Judge over 1 year ago

Submission Judgement Published

Validated

Assigned finding tags:

pendingstake-high

Prize pool breakdown

Total prize

20,000 USDC

nSLOC

609

33 USDC / LOC

High Medium

17,500 USDC

Low

2,500 USDC

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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