The Standard

Summary

Note to judges: This is separate from the known issue of "No length check for number of stake holders. This could cause a problem throughout contract if there are a high number of stakers", because this is concerned with one person being able to grief the whole system by maliciously creating a batch of dust pending stakes. This is not an issue on Arbitrum (gas limit is basically infinite). This is a critical issue if the protocol branches or redeploys to any other chain like Polygon or Ethereum. If this is a valid concern on 30M block gas limit chains and the protocol has plans to move to other chains, they should be made aware.

Discord: Dev: "...we had some plans in the past to deploy to more chains e.g. polygon...". warden: "hey, one more question regarding this, is the protocol going to be deployed on polygon, arbitrum and ethereum or". Dev: "no plans to do so any time soon, but it is possible"

If the protocol ever moves to a chain with a 30M block gas limit, a malicious user can create a batch of pending stakes and permanently clog the system, shutting down all liquidations and staking increments and decrements for all users.

Vulnerability Details

A malicious user can create ~200 pending stakes with dust amounts by calling LiquidationPool::increasePosition() 200 times through a smart contract function. It takes one day for pending stakes to mature, so every pendingStake matures at the same time. This means no matter what, they will all be clumped together whenever the first LiquidationPool::consolidatePendingStake() is called after their 24 hour incubation period. This means consolidatePendingStakes() has to update storage for each of them to transition them from pending stakes to matured positions, but will run out of gas and revert due to reaching 30M gas limit.

• Even if the details of the created pending stake are identical (sender, timestamp, amounts), each PendingStake struct still gets pushed to a unique index in the pendingStakes array.

• Inside increasePosition() after the first atomic call of 200, there will be no pending stakes to consolidate, there will be no fees to distribute, and there will be no new unique holders to add. Therefore, it is a gas efficient function that will transfer from the dust amount passed in, and just push created struct to the array.

• Importantly, consolidatePendingStakes() is a core protocol function that must never revert. The system will be in this permanently broken state because the pending stakes from the malicious user are permanently stuck in their pending state because there is no way to "flush them out". For example, LiquidationPool::deletePendingStake() is a private function only called by consolidatePendingStakes(), but because the consolidation runs out of gas and reverts, no pending stakes will be deleted because the state returns to it's previous state before the function call.

• Importantly, this will cause all liquidations and stake changes to fail protocol wide. LiquidationPoolManager::runLiquidation() calls LiqudationPool::distributeAssets() which calls consolidatePendingStake(). Also LiquidationPool::increasePosition() and LiqudationPool::decreasePosition() calls consolidatePendingStake(). All these functions will revert due to out of gas, so users can never unstake their funds and they are lost permanently, and vault liquidations are also halted.

Impact

All staked user funds permanently frozen and liquidation mechanism for vaults broken.

PoC

This example shows how a user cannot unstake via decreasePosition() after the pendingStakes DoS is made. This can be applied to any function that requires consolidatePendingStake() to be called like runLiquidation() or increasePosition() for any user.

• Install hardhat-gas-reporter for the project with npm install hardhat-gas-reporter --save-dev.

• Setup hardhat.config.js as shown below. (important to include gas limit and mocha timeout increase, otherwise test doesn't work).

• Place PoC inside liquidationPool.js and place inside of increase position and then run the specific test: npx hardhat test --grep "spam increases" . Test will run for 1 minute.

• With 170 calls to increasePosition(), which creates 170 pending stakes, there is 31,328,349 gas used when trying to call decreasePosition() because it must call consolidatePendingStakes() and update storage for each pending stake.

• NOTE: The gas spent for decreasePosition() will not show up in the gas report output unless you increase the blockGasLimit in hardhat configs to 40_000_000 and run the test again so decreasePosition() can finish execution. (When you increase gas limit to 40M the transaction will not revert since it is under the limit, but you will see the 31M gas being used for decreasePosition() in the gas report to verify it is a gas issue).

Tools Used

Manual Review

Recommendations

• Consider having a maximum number of pending stakes the system can "hold".

• Consider adding a minimum amount of tokens or value when calling increasePosition().

• Consider allowing deletePendingStake() to be callable by the owner or manager during emergency. (Meh)

• Consider having a maximum number of pending stakes per address. (May not fully fix issue because malicious user can use many different addresses).

• Consider changing the pendingStakes array into a mapping from an address to a PendingStake struct, and remove the holder member from the PendingStake struct. This would require changing increasePosition() logic for creating the pending stake, and then requires changes to consolidatePendingStakes() to accommodate a mapping instead of an array using something like enumerableMap. (Unsure of the implications of this one, just throwing it out there.)