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

Reward Eligibility Issue Due to Incorrect Holder Removal

ElHaj

Summary

  • A logic flaw in the LiquidationPool contract may cause users with pending stakes to become ineligible for rewards if they withdraw their entire staked amount before consolidation, due to premature removal from the holders array.

Vulnerability Details

  • When a user has both staked and pending stakes. The vulnerability is triggered under the following conditions:

  1. A user already has a stake recorded in the holders array (e.g., tst = 1000, euro = 200).

  2. The user decides to increase their stake, which results in additional amounts being added to pendingStakes (e.g., tst = 2000, euro = 100). The consolidatePendingStakes() function is designed to add unique holders to the holders array, so it does not add this user in this case because the user is already listed.

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
require(_tstVal > 0 || _eurosVal > 0);
>> consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
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);
}

  1. If the user then decreases their entire staked position before the pending stakes are consolidated (within one day), the empty() check within the decreasePosition() function will lead to the user being removed from the holders array.

​
function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();// this will do something and callback to distributeRewared in this contract ..
require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");// note non need for this
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]);
}
​
function deletePosition(Position memory _position) private {
>> deleteHolder(_position.holder);
delete positions[_position.holder];
}

  1. After the one-day period passes and consolidatePendingStakes() is called, the user's pending stakes are added to their position, but since they have been removed from the holders array, they are no longer eligible to get any rewards when distributeFees() or distributeAssets function called, cause in this case they won't be a holder (not in the holders array) nor they have a pending stake (it's already executed) even they are a real holder.

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;
}
}
}

Poc :

  • here a poc that shows ,how bob can be deleted from array holders ,and not take any rewards , i used foundry with a custom setup for the protocol , i didn't use the proxy pattern so i have to set some intial values in the initialize function of smartVaultManager contract :

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity ^0.8.17;
​
import "forge-std/console.sol";
import "forge-std/Test.sol";
import "../../contracts/utils/SmartVaultDeployerV3.sol";
import "../../contracts/SmartVaultManagerV5.sol";
import "../../contracts/utils/EUROsMock.sol";
import "../../contracts/utils/SmartVaultIndex.sol";
import "../../contracts/LiquidationPoolManager.sol";
import "../../contracts/LiquidationPool.sol";
import "../../contracts/SmartVaultV3.sol";
import "../../contracts/utils/ERC20Mock.sol";
import "../../contracts/utils/TokenManagerMock.sol";
import "../../contracts/utils/ChainlinkMock.sol";
contract setup is Test {
ChainlinkMock clmock;
ChainlinkMock clmock1;
TokenManagerMock tokenmanager;
SmartVaultDeployerV3 deployer;
SmartVaultManagerV5 manager;
SmartVaultIndex vaultIndex;
EUROsMock euro;
LiquidationPoolManager poolmanager;
LiquidationPool pool;
ERC20Mock tst;
ChainlinkMock clmock2;
ERC20Mock token;
address bob = makeAddr("bob");
address alice = makeAddr("alice");
//////////////////////// standard function //////////////////////////////////
function onERC721Received(address ,address ,uint ,bytes memory ) public pure returns (bytes4 retval){
return this.onERC721Received.selector;
}
receive() external payable {}
function setUp() public virtual {
skip(3 days);
// deploy chain link mock :
clmock = new ChainlinkMock("native price feed");
clmock.setPrice(160000000000);
clmock1 = new ChainlinkMock("euro price feed");
clmock1.setPrice(150000000);
// deploy the token manager :
tokenmanager = new TokenManagerMock(bytes32("native"),address (clmock));
clmock2 = new ChainlinkMock("random token price feed");
token = new ERC20Mock("token","tk",18);
token.mint(bob,10000 ether);
clmock2.setPrice(100000000);
tokenmanager.addAcceptedToken(address(token),address(clmock2));
// deploy smartVault manager :
manager = new SmartVaultManagerV5();
// deploy smart index and set the setVaultManager(manager)
vaultIndex = new SmartVaultIndex();
vaultIndex.setVaultManager(address(manager));
// deploy euro mock :
euro = new EUROsMock();
// deploy tst :
tst = new ERC20Mock("test tst","tst", 18);
// deploy smart vault deployer : and set the constaructor to (bytes32(native), address(this))
deployer = new SmartVaultDeployerV3(bytes32("native"),address(clmock1));
// intialize the custom manager :
manager.initialize(vaultIndex,address(euro),address(tokenmanager));
// deploy the pool manager :
poolmanager = new LiquidationPoolManager(address(tst),address(euro),address(manager),address(clmock1), payable (address(this)),50000);
pool = LiquidationPool(poolmanager.pool());
// set some initial values to manager :
manager.setSmartVaultDeployer(address(deployer));
euro.grantRole(euro.DEFAULT_ADMIN_ROLE(),address(manager));
manager.setLiquidatorAddress(address(poolmanager));
manager.setMintFeeRate(10000);
manager.setBurnFeeRate(5000);
manager.setSwapFeeRate(5000);
manager.setProtocolAddress(address(poolmanager));
vm.deal(address(this), 20 ether);
}
function test_removedHolder() public {
tst.mint(alice,100 ether);
tst.mint(bob,1100 ether);
vm.prank(bob);
tst.approve(address(pool),type(uint).max);
vm.prank(alice);
tst.approve(address(pool),type(uint).max);
// bob first stake some tst:
vm.prank(bob);
pool.increasePosition(100 ether,0);
vm.prank(alice);
pool.increasePosition(50 ether,0);
skip(2 days);
// bob increase his position :
vm.startPrank(bob);
pool.increasePosition(100 ether,0);
// before 1 day pass bob remove his first deposit :
(LiquidationPool.Position memory bobPosition ,)= pool.position(bob);// bob position before rewards
(LiquidationPool.Position memory alicePosition ,)= pool.position(alice);// alice position before rewards
pool.decreasePosition(100 ether,0);
skip(2 days);vm.stopPrank();
// lets say another user stakes which will trigger consolidatePendingStakes() (alice for simplicity)
vm.prank(alice);
pool.increasePosition(50 ether,0);
// mint some euro to the LiquidationPoolManager , and distribute rewards
euro.mint(address(poolmanager), 100 ether);
poolmanager.distributeFees();
(LiquidationPool.Position memory bobPositionAfter ,)= pool.position(bob);
(LiquidationPool.Position memory alicePositionAfter ,)= pool.position(alice);
console.log("- bob stakes before :",bobPosition.TST);
console.log("- while alice stakes before is ", alicePosition.TST, " you can see that bob have more stakes then alice");
console.log("- now bob got ",bobPositionAfter.EUROs," euro rewards ");
console.log("- while alice had : ", alicePositionAfter.EUROs, " euro rewards ,even thougth bob have way more stake then her");
}
}

  • console after running test :

Running 1 test for test/foundry_tests/setup.t.sol:setup
[PASS] test_removedHolder() (gas: 839224)
Logs:
- bob stakes before : 200000000000000000000
- while alice stakes before is 50000000000000000000 you can see that bob have more stakes then alice
- now bob got 0 euro rewards
- while alice had : 50000000000000000000 euro rewards ,even thougth bob have way more stake then her

Impact

    • This flaw can result in a user being unfairly excluded from reward distributions, despite having a pending stake that should qualify them for rewards.

Tools Used

vs code

Recommendations

  • It is recommended to remove the addUniqueHolder() function call from the increasePosition() function, as its current placement is unnecessary. When a holder is unique, it implies their position is empty, and thus they are not entitled to rewards at that moment. To ensure accurate tracking and reward eligibility, the addUniqueHolder() function should be called within the consolidatePendingStakes() function after pending stakes have been added to the holder's position.

// add addUniqueHolder();
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;//note no need for storing this ..
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));// @note : tooo heavy compitation ..
++ addUniqueHolder(_stake.holder);
i--;
}
}
}
​
// remove addUniqueHolder();
function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
//prev code ....
-- addUniqueHolder(msg.sender);
}
​
​
​
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.