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

Fees denied to protocol/holders & funds are stuck for various combinations of holders and fees

t0x1c

Summary

LiquidationPool::distributeFees() uses safeTransferFrom() to pull fees to be distributed to holders (the _amount parameter of function distributeFees()). However, rounding-down of the calculations may result in leftover fee remaining in the LiquidationPool contract with no way to rescue them, causing them to be stuck forever:

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

Example 1

Let us see an example where

  • the accumulated EUROs fee is (Equal to 198)

  • which means that with a 50% fee rate, needs to be distributed (the _amount) among the holders

  • we will also suppose that the number of holders is

  • with each holder having staked an equal quantity of TST equal to .

Hence,

For each holder, L188 will calculate to:
$$
\begin{equation}
\begin{split} positions[_holder].EUROs &= (_amount * positions[_holder].TST) \div \text{tstTotal} \
&= (99 * 50e18) \div {5000e18} \
&= 4950e18 \div {5000e18} \
&= 0 \text{ (rounding down by solidity)}
\end{split}
\end{equation}
$$

Hence, all of the _amount ( ) now sits idle, undistributed within the contract with no way to rescue them.


Example 2

Another example where

  • the accumulated fee is (Equal to 40e18)

  • which means that with a 50% fee rate, needs to be distributed (the _amount) among the holders

  • we will also suppose that the number of holders is

  • with each holder having staked an equal quantity of TST equal to .

Hence,

For each holder, L188 will calculate to:
$$
\begin{equation}
\begin{split} positions[_holder].EUROs &= (_amount * positions[_holder].TST) \div \text{tstTotal} \
&= (20e18 \text{ EUROs} * 50e18) \div {150e18} \
&= 6666666666666666666 \text{ EUROs} \
\end{split}
\end{equation}
$$

which now sits idle, undistributed within the contract with no way to rescue them.


Note that this issue may arise

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);
}
​
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]);
}
​

PoC

We will recreate example-1 but with holders and accumulated fee as .

Apply the following patch inside file test/liquidationPool.js and run via npx hardhat test --grep "results in stuck funds when holders are many & accumulated fee is low" to see the test pass:

diff --git a/test/liquidationPool.js b/test/liquidationPool_2.js
index 8dd8580..05a74db 100644
--- a/test/liquidationPool.js
+++ b/test/liquidationPool_2.js
@@ -7,8 +7,9 @@ describe('LiquidationPool', async () => {
let user1, user2, user3, Protocol, LiquidationPoolManager, LiquidationPool, MockSmartVaultManager,
ERC20MockFactory, TST, EUROs;
+ let user4, user5, user6, user7, user8, user9, user10;
beforeEach(async () => {
- [ user1, user2, user3, Protocol ] = await ethers.getSigners();
+ [ user1, user2, user3, Protocol, user4, user5, user6, user7, user8, user9, user10 ] = await ethers.getSigners();
ERC20MockFactory = await ethers.getContractFactory('ERC20Mock');
TST = await ERC20MockFactory.deploy('The Standard Token', 'TST', 18);
EUROs = await (await ethers.getContractFactory('EUROsMock')).deploy();
@@ -224,6 +225,94 @@ describe('LiquidationPool', async () => {
expect(_position.EUROs).to.equal(distributedFees2);
});
+ it('results in stuck funds when holders are many & accumulated fee is low', async () => {
+ expect(await EUROs.balanceOf(LiquidationPool.address)).to.equal(0);
+
+ const tstStake = ethers.utils.parseEther('10');
+ await TST.mint(user1.address, tstStake);
+ await TST.connect(user1).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user2.address, tstStake);
+ await TST.connect(user2).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user3.address, tstStake);
+ await TST.connect(user3).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user4.address, tstStake);
+ await TST.connect(user4).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user5.address, tstStake);
+ await TST.connect(user5).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user6.address, tstStake);
+ await TST.connect(user6).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user7.address, tstStake);
+ await TST.connect(user7).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user8.address, tstStake);
+ await TST.connect(user8).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user9.address, tstStake);
+ await TST.connect(user9).approve(LiquidationPool.address, tstStake);
+ await TST.mint(user10.address, tstStake);
+ await TST.connect(user10).approve(LiquidationPool.address, tstStake);
+
+ await LiquidationPool.connect(user1).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user2).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user3).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user4).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user5).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user6).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user7).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user8).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user9).increasePosition(tstStake, 0);
+ await LiquidationPool.connect(user10).increasePosition(tstStake, 0);
+
+ await fastForward(DAY);
+ // @audit-info : a `FEE-ACCUMULATING-ACTION` like mint() causes credit of fees into the LiquidationPoolManager contract
+ const fees = 18;
+ await EUROs.mint(LiquidationPoolManager.address, fees);
+
+ // someone calls distributeFees()
+ await LiquidationPoolManager.distributeFees();
+
+ let { _position } = await LiquidationPool.position(user1.address);
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user2.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user3.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user4.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user5.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user6.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user7.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user8.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user9.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ ({ _position } = await LiquidationPool.position(user10.address));
+ expect(_position.TST).to.equal(tstStake);
+ expect(_position.EUROs).to.equal(0); // @audit : no EUROs credited from the accrued fee
+
+ // @audit : all fees now stuck in the LiquidationPool contract
+ expect(await EUROs.balanceOf(LiquidationPool.address)).to.equal(fees/2);
+ });
+
it('does not allow decreasing beyond position value, even with assets in pool', async () => {
const tstStake1 = ethers.utils.parseEther('10000');
await TST.mint(user1.address, tstStake1);

Impact

  • Loss of fee

  • Funds get stuck

Tools Used

Manual inspection, Hardhat.

Recommendations

There are multiple ways to handle this. One approach would be to return any leftover fee in the LiquidationPool.sol contract back to the LiquidationPoolManager.sol contract which then gets claimed as protocol fee.

Apply the following patch to contracts/LiquidationPool.sol which updates the distributeFees() function:

diff --git a/contracts/LiquidationPool.sol b/contracts/LiquidationPool2.sol
index 9b8e593..011d568 100644
--- a/contracts/LiquidationPool.sol
+++ b/contracts/LiquidationPool2.sol
@@ -183,12 +183,22 @@ contract LiquidationPool is ILiquidationPool {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
+ uint256 actuallyDistributed = 0;
+ uint256 feeToDistribute = 0;
for (uint256 i = 0; i < holders.length; i++) {
address _holder = holders[i];
- positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
+ feeToDistribute = _amount * positions[_holder].TST / tstTotal;
+ positions[_holder].EUROs += feeToDistribute;
+ actuallyDistributed += feeToDistribute;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
- pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
+ feeToDistribute = _amount * pendingStakes[i].TST / tstTotal;
+ pendingStakes[i].EUROs += feeToDistribute;
+ actuallyDistributed += feeToDistribute;
+ }
+ uint256 leftover = _amount - actuallyDistributed;
+ if (leftover > 0) {
+ IERC20(EUROs).transfer(msg.sender, leftover);
}
}
}
Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

t0x1c Submitter
almost 2 years ago
hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

precision-distributeFees

Support

FAQs

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