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

`LiquidationPoolManager::distributeFees()` sends whole balance to protocol after single call, also lacks AC

NentoR

Summary

LiquidationPoolManager::distributeFees() is callable by anyone when it should only be callable from LiquidationPoolManager and LiquidationPool. This leads to any user being able to deprive it of its EUROs balance at any time. There also seems to be a problem with the implementation, after a single distribution, fees are sent to the liquidation pool but the whole balance is sent to the protocol afterwards, leaving it without balance for subsequent calls.

Vulnerability Details

LiquidationPoolManager::distributeFees() is susceptible to the following problems:

  • It sends the whole balance to the protocol after a single run. This leads to the problem where it won't have enough balance to send to the liquidation pool and will have to be refilled after each call. While this doesn't lead to any malfunction of the liquidation pool, it seems like a badly designed implementation.

  • Callable by anyone - Anyone is able to execute it at any time when it is supposed to be only accessed by LiquidationPoolManager::runLiquidation(), LiquidationPool::increasePosition(), and LiquidationPool::decreasePosition(),

POC (test/liquidationPool.js, make sure to also destructure HUNDRED_PC from ./common):

describe("LiquidationPoolManager distributeFees", async () => {
it("loses balance after single call", async () => {
// Staker 1
const tstStake1 = ethers.utils.parseEther('10000')
await TST.mint(user1.address, tstStake1)
await TST.approve(LiquidationPool.address, tstStake1)
await LiquidationPool.increasePosition(tstStake1, 0)
​
// Staker 2
const tstStake2 = ethers.utils.parseEther('20000')
await TST.mint(user2.address, tstStake2)
await TST.connect(user2).approve(LiquidationPool.address, tstStake2)
await LiquidationPool.connect(user2).increasePosition(tstStake2, 0)
​
// Distribute fee
const feeToDistribute = ethers.utils.parseEther('1000')
EUROs.mint(LiquidationPoolManager.address, feeToDistribute)
expect(await EUROs.balanceOf(LiquidationPoolManager.address)).to.equal(feeToDistribute)
await LiquidationPoolManager.distributeFees()
​
// EUROs balance has increased for all positions after distribution
const tstTotal = await TST.balanceOf(LiquidationPool.address)
const feesForPool = feeToDistribute.mul(POOL_FEE_PERCENTAGE).div(HUNDRED_PC)
const newPosition1Amount = feesForPool.mul(tstStake1).div(tstTotal)
const newPosition2Amount = feesForPool.mul(tstStake2).div(tstTotal)
const { _position: pos1 } = await LiquidationPool.position(user1.address)
const { _position: pos2 } = await LiquidationPool.position(user2.address)
expect(pos1.EUROs).to.equal(newPosition1Amount)
expect(pos2.EUROs).to.equal(newPosition2Amount)
​
// No EUROs left in LiquidationPoolManager
expect(await EUROs.balanceOf(LiquidationPoolManager.address)).to.equal(0)
​
// Perform second distribution
await LiquidationPoolManager.distributeFees()
​
// EUROs balance hasn't increased for positions after second distribution
const { _position: pos1SecondDistribution } = await LiquidationPool.position(user1.address)
const { _position: pos2SecondDistribution } = await LiquidationPool.position(user2.address)
expect(pos1SecondDistribution.EUROs).to.equal(pos1.EUROs)
expect(pos2SecondDistribution.EUROs).to.equal(pos2.EUROs)
})
})

Impact

LiquidationPoolManager is left with no balance after a single call to LiquidationPoolManager::distributeAssets(). Also, it is callable by anyone when it should be restricted to only LiquidationPoolManager and LiquidationPool.

Tools Used

Manual Analysis

Recommendations

  • Limit access control of this function to only LiquidationPoolManager and LiquidationPool

  • Re-think the logic of the function. It doesn't make sense for it to send all of its balance to the protocol after a single call.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

Support

FAQs

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