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

Potential Loss of Rewards Due to Stake Withdrawal in `LiquidationPool`

ua1552

Summary

When a staker un-stakes during an active pending stake, there is a vulnerability in the LiquidationPool contract that leads to a loss of liquidation rewards and reward fees. The issue arises when the LiquidationPool::decreasePosition() function is called, and there is an active PendingStake for the staker. The staker's address is removed from the holders array, causing problems during reward consolidation.

Vulnerability Details

The problem lies in the LiquidationPool::decreasePosition() function, specifically in the check at the end of the function that removes the staker's address from the holders array if their position is empty. This becomes problematic when there is an active PendingStake for the staker. As a result, during reward consolidation, the staker's address is not added back to the holders array and also removed from the pendingStakes, impacting the calculation and distribution of rewards.

LIquidationPool::decreasePosition(...)

function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
if (_tstVal > 0) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}
@> if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}

GitHub: 149-162

The LiqudiationPool::consolidatePendingStakes() function is responsible for consolidating pending stakes, but it fails to add the staker back to the holders array, leading to the loss of rewards. The only way for the staker to be added to the array is by depositing tokens again using the LiquidationPool::increasePosition() function.

function consolidatePendingStakes() private {
uint256 deadline = block.timestamp - 1 days;
for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
PendingStake memory _stake = pendingStakes[uint256(i)];
if (_stake.createdAt < deadline) {
positions[_stake.holder].holder = _stake.holder;
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));
// pause iterating on loop because there has been a deletion. "next" item has same index
i--;
}
}
}

Additionally, the LiquidationPool::distributeFees() and LiquidationPool::distributeAssets() functions depend on both the holders array and pendingStakes array to distribute rewards. Since the staker's address is not present in both arrays after stake consolidation, they do not receive any rewards.

LiquidationPool::distributeFees(...)

function distributeFees(uint256 _amount) external onlyManager {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
@> for (uint256 i = 0; i < holders.length; i++) {
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
}
}
}

GitHub: 182-194

LiquidationPool::distributeAssets(...)

function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
uint256 stakeTotal = getStakeTotal();
uint256 burnEuros;
uint256 nativePurchased;
@> for (uint256 j = 0; j < holders.length; j++) {
​
...
​
}
}

GitHub: 205

Let's take an example to understand it better:

  1. There are three users alice, bob and mike.

  2. alice and bob both has 1000 EUROs and 1000 TSTs. Both deposits that into the LiquidationPool. So now, there are 2000 EUROs and 2000 TSTs is in pending stakes.

  3. Now mike comes and decides to create a SmartVault. So he calls the SmartVaultManager and creates one for himself. Now whenever he mints or burns or swaps any token, there will be fee he will have to pay that will be deposited to the LiquidationPoolManager that will ultimately be given to Pool Staker (i.e is alice and bob).

  4. Now let's say 1 week has passed and both bob's and alice's stakes has been consolidated and they have earn some fee rewards and their new balance is 1010 EURO and 1000 TSTs each.

  5. bob stakes 1000 EUROs more. But on the same day he decides to unstake his consolidated stakes due to some reasons. So he does that and removes 1010 EUROs and 1000 TSTs from the pool. But doing this has removed his address from the holders mapping as the LiquidationPool::empty() will return true and his address will be removed from the array. Now he has 1000 EUROs and 1000 TSTs in pending stakes.

  6. Now he will keep earning his fee rewards on pending stakes. But when his pending stakes in consolidated after the deadline. his amount will be added to the positions mapping only and will not be added to the holders array. And that means any reward after that will not be sent to him as a staker needs to be present in the holders array to earn rewards as we have seen above. And hence loss of rewards for him as long as he does not stakes again.

Impact

Staker will lose rewards.

Proof Of Concept

NOTE
The below given test is written in the foundry framework instead of hardhat. Please go to the repo given below and follow the steps mentioned in the readme to setup the project. Every tests for the current codebase is given in the repo itself.
Github: Repo

Here is a test for PoC that demonstarates the loss of fee rewards:

Original Test: [Link]

function test_decreaseStakeCanCauseLossOfFeeReward() public {
////////////////////////////////////////////
/// Minting some tokens to users ///
////////////////////////////////////////////
​
uint256 amount = 1000 ether; // 1000 tokens
​
tokens.eurosToken.mint(alice, amount * 2);
tokens.eurosToken.mint(bob, amount * 2);
​
tokens.tstToken.mint(alice, amount * 2);
tokens.tstToken.mint(bob, amount * 2);
​
/////////////////////////////////////////////////////////
/// Alice and Bob deposits tokens in the pool ///
/////////////////////////////////////////////////////////
​
vm.startPrank(alice);
tokens.tstToken.approve(address(contracts.liquidationPool), amount);
tokens.eurosToken.approve(address(contracts.liquidationPool), amount);
vm.stopPrank();
​
vm.startPrank(bob);
tokens.tstToken.approve(address(contracts.liquidationPool), amount);
tokens.eurosToken.approve(address(contracts.liquidationPool), amount);
vm.stopPrank();
​
vm.prank(alice);
contracts.liquidationPool.increasePosition(amount, amount);
​
vm.prank(bob);
contracts.liquidationPool.increasePosition(amount, amount);
​
////////////////////////////////////////////////////////////
///// Checking if correct value has been deposited /////
////////////////////////////////////////////////////////////
​
(LiquidationPool.Position memory alicePosition,) = contracts.liquidationPool.position(alice);
(LiquidationPool.Position memory bobPosition,) = contracts.liquidationPool.position(bob);
​
assertEq(alicePosition.EUROs, amount, "Alice's euros amount are not eqaul");
assertEq(alicePosition.TST, amount, "Alice's TSTs amount are not eqaul");
assertEq(bobPosition.EUROs, amount, "Bob's euros amount are not eqaul");
assertEq(bobPosition.TST, amount, "Bob's TSTs amount are not eqaul");
​
/////////////////////////////////////////////////////////
/// Skipping some time to consolidate stakes ////
/////////////////////////////////////////////////////////
​
// NOTE: Actual consolidation will be done when someone increases or decreases their positions
skip(block.timestamp + 1 weeks);
​
/////////////////////////////////////////////////////////////////////////////
/// Alice and bob holds 1000 TST and EUROs Each in Pending Stakes ///
/////////////////////////////////////////////////////////////////////////////
​
////////////////////////////////////
/// Mike Creates a Vault ///
////////////////////////////////////
​
address mike = makeAddr("mike");
(address mikesSmartVault, uint256 tokenIdMinted) = _createSmartVault(mike);
​
//////////////////////////////////////////////////////////
/// Checking If Mike is the owner of the vault ///
//////////////////////////////////////////////////////////
​
assertEq(contracts.smartVaultManagerV5.ownerOf(tokenIdMinted), mike, "Mike is not the owner");
​
////////////////////////////////////////////////////////
/// Mike deposits some tokens in the vault //////
////////////////////////////////////////////////////////
​
vm.startPrank(mike);
uint256 mikePaxgAmount = 1000 ether;
tokens.paxgToken.mint(mike, mikePaxgAmount);
tokens.paxgToken.transfer(mikesSmartVault, amount);
vm.stopPrank();
​
//////////////////////////////////////////////////////////////////////////////////
/// Checking if the correct balances has been transferred to the vault ///
//////////////////////////////////////////////////////////////////////////////////
​
uint256 mikePaxgBalanceInVault = tokens.paxgToken.balanceOf(mikesSmartVault);
​
assertEq(mikePaxgAmount, mikePaxgBalanceInVault, "Balance is not equal");
​
/////////////////////////////////////////////////////////////////////////////////////////////////////
/// Mike decides to mint some euros from the vault that generates some fees ///
/// feeGeneratedFromMint = (eurosMikeWant * constants.PROTOCOL_FEE_RATE) / 100000 ///
/// feeGenereatedFromMint = 500000000000000000000 * 500 / 100000 ///
/// feeGeneratedFromMint = 2.5 ether (i.e 2.5 tokens) ///
/// feeShareForPool = feeGeneratedFromMint * constants.POOL_FEE_PERCENTAGE) / 100000 ///
/// feeShareForPool = 2.5 ether * 50000 / 100000 ///
/// feeShareForPool = 1.25 ether or 1250000000000000000 ///
/////////////////////////////////////////////////////////////////////////////////////////////////////
​
vm.startPrank(mike);
uint256 eurosMikeWant = mikePaxgBalanceInVault / 2;
SmartVaultV3(payable(mikesSmartVault)).mint(mike, eurosMikeWant);
vm.stopPrank();
​
/////////////////////////////////////////////////////////////////////////////////////////
/// Checking if the correct amount of euros has been minted from the vault //////
/////////////////////////////////////////////////////////////////////////////////////////
​
uint256 mikeEurosBalance = tokens.eurosToken.balanceOf(mike);
assertEq(mikeEurosBalance, eurosMikeWant, "Balance is not equal");
​
///////////////////////////////////////////////////////////////////////////
/// There should be some euros fee in the liqudiationPoolManager ///
///////////////////////////////////////////////////////////////////////////
​
uint256 feeGenerated = (eurosMikeWant * constants.PROTOCOL_FEE_RATE) / 100000;
uint256 eurosFeeInLiquidationManager = tokens.eurosToken.balanceOf(address(contracts.liquidationPoolManager));
assertEq(eurosFeeInLiquidationManager, feeGenerated, "fee is not equal");
assertEq(eurosFeeInLiquidationManager, 2.5 ether, "fee is not equal");
​
///////////////////////////////////////////////////////////////////////////////////////////////////
/// Bob add more tokens to his positions. This time there is a fee ///
/// in the liquidationPoolManager that will be added to his stakes ///
/// Proporational to his stakes ///
/// Bob's fee share = feeShareForPool * (balance before the stake) / (Total TST balance) ///
/// Bob's fee share = 1.25 ether * 1000 tokens / 2000 tokens (1000 bobs and 1000 alice's) ///
/// Bob's fee share = 1250000000000000000 * 1000 / 2000 tokens ///
/// Bob's fee share = 0.625 euros or 625000000000000000 ///
///////////////////////////////////////////////////////////////////////////////////////////////////
​
(bobPosition,) = contracts.liquidationPool.position(bob);
​
uint256 bobsFeeShare = ((eurosFeeInLiquidationManager * constants.POOL_FEE_PERCENTAGE / 100000 ) * bobPosition.TST) / tokens.tstToken.balanceOf(address(contracts.liquidationPool));
​
vm.startPrank(bob);
tokens.eurosToken.approve(address(contracts.liquidationPool), amount);
tokens.tstToken.approve(address(contracts.liquidationPool), amount);
contracts.liquidationPool.increasePosition(amount, amount);
vm.stopPrank();
​
////////////////////////////////////////////////////////////////////////////////////////////
/// Checking if the correct value has been deposited to the pool and bob's position ///
////////////////////////////////////////////////////////////////////////////////////////////
​
(bobPosition,) = contracts.liquidationPool.position(bob);
assertEq(bobPosition.EUROs, amount * 2 + bobsFeeShare, "Bob's euros amount are not eqaul");
assertEq(bobsFeeShare, 0.625 ether, "Bob's fee share is not equal");
assertEq(bobPosition.TST, amount * 2, "Bob's TSTs amount are not eqaul");
​
////////////////////////////////////////////////////////////////////////////////////////
/// Bob now holds 1000 TSTs and 1000.0625 euros in consolidated stakes ////
/// and 1000 TSTs and 1000 euros in pending stakes ////
////////////////////////////////////////////////////////////////////////////////////////
​
///////////////////////////////////////////////////////////////////////
/// Bob decides to withdraw his stake for some reason ///
/// on same day. Since he deposited the tokens on the ///
/// same day, he can only withdraw his consolidated stake. ///
/// Bob's consolidated stake = 1000 Tst and 1000.0625 euros ///
/// Since he is withdrawing all of his consolidated stake, ///
/// his address will be removed from holders array. ///
///////////////////////////////////////////////////////////////////////
​
vm.startPrank(bob);
contracts.liquidationPool.decreasePosition(amount, amount + 0.625 ether);
vm.stopPrank();
​
////////////////////////////////////////////////////////////////////////////////
/// Checking bob's position. There is only balance in pending stakes ///
/// Bob's consolidated stake = 0 Tst and 0 euros ///
/// Bob's pending stake = 1000 Tst and 1000 euros ///
////////////////////////////////////////////////////////////////////////////////
​
(bobPosition,) = contracts.liquidationPool.position(bob);
​
assertEq(bobPosition.EUROs, amount, "Bob's euros amount are not eqaul");
assertEq(bobPosition.TST, amount, "Bob's euros amount are not eqaul");
​
//////////////////////////////////////////////////////////////////////////////////////
/// Mike decides to mint some more tokens against his vault balances. ///
/// More fee will be generated and added to the liquidationPoolManager ///
/// feeGenereatedFromMint = 2.5 tokens (i.e because same amount is minted) ///
/// feeShareForPool = 1.25 ether or 1250000000000000000 ///
//////////////////////////////////////////////////////////////////////////////////////
​
tokens.paxgToken.mint(mike, amount);
vm.startPrank(mike);
tokens.paxgToken.transfer(mikesSmartVault, amount);
SmartVaultV3(payable(mikesSmartVault)).mint(mike, eurosMikeWant);
vm.stopPrank();
​
///////////////////////////////////////////////////////////////////////////
/// Checking if correct balance has been deposited to the vault ///
///////////////////////////////////////////////////////////////////////////
​
mikePaxgBalanceInVault = tokens.paxgToken.balanceOf(mikesSmartVault);
​
assertEq(
mikePaxgAmount * 2, // because mike deposited 1000 tokens before
mikePaxgBalanceInVault,
"Balance is not equal"
);
​
////////////////////////////////////////////////////
/// Checking if correct fee is generated ///
/// Fee generated will be same as before ///
/// as the amount minted is same ///
////////////////////////////////////////////////////
​
eurosFeeInLiquidationManager = tokens.eurosToken.balanceOf(address(contracts.liquidationPoolManager));
​
assertEq(eurosFeeInLiquidationManager, feeGenerated, "Balance is empty");
assertEq(eurosFeeInLiquidationManager, 2.5 ether, "Balance is empty");
​
///////////////////////////////////////////////////////////////////////////////
/// Getting bob's position to check if his amount shows the rewards ///
///////////////////////////////////////////////////////////////////////////////
​
(bobPosition,) = contracts.liquidationPool.position(bob);
​
bobsFeeShare = ((eurosFeeInLiquidationManager * constants.POOL_FEE_PERCENTAGE / 100000 ) * bobPosition.TST) / (tokens.tstToken.balanceOf(address(contracts.liquidationPool)));
​
// NOTE: here the bobPosition.EUROs shows the wrong amount since there is a bug in position function. It is calculating fee
// of staker based on the total balance of manager while pool is going to receive only percentage of the fee generated. Rest
// will go to protocol address. In this case pool fee percentage is 50% so the amount shown by bobPosition.EUROs will be
// twice the amount he will receive.
​
assertEq(bobPosition.EUROs, amount + (0.625 ether * 2), "Bob's euros amount are not eqaul");
assertEq(bobPosition.EUROs, amount + (bobsFeeShare * 2), "Bob's euros amount are not eqaul");
assertEq(bobPosition.TST, amount, "Bob's euros amount are not eqaul");
​
/////////////////////////////////////////////////////
/// Skipping some time to consolidate stakes ///
/////////////////////////////////////////////////////
​
​
skip(block.timestamp + 1 weeks);
​
​
///////////////////////////////////////////////////////////////////////////////////////////
/// Since enough time has passed bob decides to withdraw rest of his amount. ///
/// Also there is a fee in liquidationPoolManager that will be added to his ///
/// stakes corresponding to his stake ratio. ///
/// Bob's fee share = 0.625 (since both fee generated and stake is same) ///
///////////////////////////////////////////////////////////////////////////////////////////
​
vm.startPrank(bob);
contracts.liquidationPool.decreasePosition(amount, amount);
vm.stopPrank();
​
/////////////////////////////////////////////////////////////////////////////////
/// Since bob has withdrawn all of his stakes excluding fee received, ///
/// there should be amount equals to his fee share in the position. ///
/// That means bob's position should be 0 TST and 0.625 euros ///
/////////////////////////////////////////////////////////////////////////////////
​
(bobPosition,) = contracts.liquidationPool.position(bob);
​
///////////////////////////////////////////////////////////////////////////////////////////////////
/// But fee is not received since bob's address has been withdrawn from the holders array ///
/// So his position should be 0 TST and 0 euros and all of the fee should have been ///
/// transferred to alice ///
/// (i.e 0.625 euros of first fee rewards and 1.25 euros of second fee reward) ///
///////////////////////////////////////////////////////////////////////////////////////////////////
​
assertEq(bobPosition.EUROs, 0, "Bob's euros amount is not zero");
assertEq(bobPosition.TST, 0);
​
(alicePosition,) = contracts.liquidationPool.position(alice);
assertEq(alicePosition.EUROs, amount + (bobsFeeShare * 3), "Alice's euros amount are not eqaul");
​
}

Output:

AAMIR@Victus MINGW64 /d/revolution-audit-foundry (master)
$ forge test --mt test_decreaseStakeCanCauseLossOfFeeReward
[⠢] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/unit/Unit.t.sol:Unit
[PASS] test_decreaseStakeCanCauseLossOfFeeReward() (gas: 4045891)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.22ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

  • Manual Review

Recommendations

It is recomended to add one of the following ways to solve the issue:

  1. If there is a pending stake for the staker, do not remove him from the holders durning the decreasePosition. Only decrease his position amount.

  2. Call LiquidationPool::addUniqueHolder(...) in the LiquidationPool::consolidatePendingStakes.

function consolidatePendingStakes() private {
uint256 deadline = block.timestamp - 1 days;
for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
PendingStake memory _stake = pendingStakes[uint256(i)];
if (_stake.createdAt < deadline) {
positions[_stake.holder].holder = _stake.holder;
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));
​
+ addUniqueHolder(_staker.holder);
// pause iterating on loop because there has been a deletion. "next" item has same index
i--;
​
​
}
}
}
Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

pendingstake-dos

ua1552 Submitter
almost 2 years ago
hrishibhat Lead Judge almost 2 years 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.

Give us feedback!