Summary
If a user with an existing position and increases it, it will ends up in the PendingStake and then he calls decreasePosition() for the entire initial position, they will no longer be in the holders array.
Vulnerability Details
If Bob has a position and increases it, he will have a PendingStake that will take 1 day to be added to his position. However, if he decreases his position entirely in the meantime, he will be removed from the holders array while still having a pending stack. This is because the decreasePosition() function checks if Bob's position is empty without verifying if he has a PendingStake:
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]);
}
The deletePosition() function will remove Bob's position and delete him from the array by calling deleteHolder:
function deletePosition(Position memory _position) private {
deleteHolder(_position.holder);
delete positions[_position.holder];
}
function deleteHolder(address _holder) private {
for (uint256 i = 0; i < holders.length; i++) {
if (holders[i] == _holder) {
holders[i] = holders[holders.length - 1];
holders.pop();
}
}
}
However, when someone calls a function that calls consolidateStack(), their PendingStack will be added, and they will no longer be counted in the Rewards:
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));
i--;
}
}
}
Moreover, this distorts the calculation of getStackTotal() and getTstTotal(). When the consolidateStack function is called, it adds the pending stack to the position without adding it to the holders.
Impact
The user is completely excluded from the LiquidationPool Reward system. He's Euro position will not be take into account and his euro token will not be burn. It's a loss for the user and the protocol. other holders will also have to pay more euros and will receive less rewards.
Proof of Concept
To run this POC you will need to follow few steps in order to add foundry in the hardhat project:
1-run npm i --save-dev @nomicfoundation/hardhat-foundry
2-Add require("@nomicfoundation/hardhat-foundry"); to the top of your hardhat.config.js file.
(if the git repository is not initialized rungit init in your terminal before)
3-Run npx hardhat init-foundry in your terminal. This will generate a foundry.toml file based on your Hardhat project's existing configuration, and will install the forge-std library.
After that create a file in the test folder with the extension .sol
pragma solidity ^0.8.17;
import {LiquidationPoolManager} from "../contracts/LiquidationPoolManager.sol";
import {ERC20Mock} from "../contracts/utils/ERC20Mock.sol";
import {EUROsMock} from "../contracts/utils/EUROsMock.sol";
import {ChainlinkMock} from "../contracts/utils/ChainlinkMock.sol";
import {MockSmartVaultManager} from "../contracts/utils/MockSmartVaultManager.sol";
import {LiquidationPool} from "../contracts/LiquidationPool.sol";
import {TokenManagerMock} from "../contracts/utils/TokenManagerMock.sol";
import {Test, console} from "../lib/forge-std/src/Test.sol";
contract POCLiquidation is Test {
bytes32 NATIVE = bytes32(bytes("ETH"));
ChainlinkMock ETHUsd;
MockSmartVaultManager vaultManager;
LiquidationPoolManager poolManager;
EUROsMock euros;
ERC20Mock TST;
ERC20Mock WBTC;
ChainlinkMock EurUsd;
ChainlinkMock BTCUsd;
TokenManagerMock tokenManager;
LiquidationPool pool;
address protocol = makeAddr("protocol");
function setUp() public {
vm.warp(1 days);
TST = new ERC20Mock("The Standard Token", "TST", 18);
ETHUsd = new ChainlinkMock("ETH / USD");
ETHUsd.setPrice(2000e8);
tokenManager = new TokenManagerMock(NATIVE, address(ETHUsd));
euros = new EUROsMock();
EurUsd = new ChainlinkMock("EUR/USD");
EurUsd.setPrice(108e6);
WBTC = new ERC20Mock("wrapped BTC", "WBTC", 8);
BTCUsd = new ChainlinkMock("WBTC/USD");
BTCUsd.setPrice(3500000000000);
tokenManager.addAcceptedToken(address(WBTC), address(BTCUsd));
vaultManager = new MockSmartVaultManager(110000, address(tokenManager));
poolManager = new LiquidationPoolManager(
address(TST), address(euros), address(vaultManager), address(EurUsd), payable(protocol), 50000
);
address poolAddress = poolManager.pool();
pool = LiquidationPool(poolAddress);
euros.grantRole(euros.DEFAULT_ADMIN_ROLE(), address(vaultManager));
euros.grantRole(euros.MINTER_ROLE(), address(this));
euros.grantRole(euros.BURNER_ROLE(), address(this));
euros.grantRole(euros.BURNER_ROLE(), address(pool));
}
function test_userIsNotInTheHoldersArray() public {
address bob = makeAddr("Bob");
address alice = makeAddr("Alice");
euros.mint(bob, 200e18);
TST.mint(bob, 200e18);
euros.mint(alice, 100e18);
TST.mint(alice, 100e18);
vm.startPrank(bob);
euros.approve(address(pool), 100e18);
TST.approve(address(pool), 100e18);
pool.increasePosition(100e18, 100e18);
vm.stopPrank();
vm.warp(block.timestamp + 1 days + 1);
vm.startPrank(alice);
euros.approve(address(pool), 100e18);
TST.approve(address(pool), 100e18);
pool.increasePosition(100e18, 100e18);
vm.stopPrank();
vm.startPrank(bob);
euros.approve(address(pool), 100e18);
TST.approve(address(pool), 100e18);
pool.increasePosition(100e18, 100e18);
pool.decreasePosition(100e18, 100e18);
vm.stopPrank();
vm.warp(block.timestamp + 1 days + 1);
vm.startPrank(alice);
pool.decreasePosition(100e18, 100e18);
vm.expectRevert();
pool.position(bob);
}
}
That the setUp is exactly the same that the one in the LiquidationPoolManager unit tests except that I deploy only the WBTC mock.
Copy this code in the file and run the command : forge test --mt test_userIsNotInTheHoldersArray --via-ir
Tools Used
Manual review
Recommendations
The decreasePosition() function should check if the user has a pending stack before removing their position and removing them from the holders.
if (empty(positions[msg.sender])&& holderPendingStakes(msg.sender)==0) deletePosition(positions[msg.sender]);
