nevillehuang
commented
7 months ago Invalid, lack required PoC for gas consumption issues required by sherlock
Closed sherlock-admin2 closed 7 months ago
nevillehuang
commented
7 months ago Invalid, lack required PoC for gas consumption issues required by sherlock
FindEverythingX
medium
DoS within settle logic due to unbatched loop (possible without intentional griefing, solely during normal business operation)
Summary
DoS within settle logic due to unbatched loop (possible without intentional griefing, solely during normal business operation)
Vulnerability Detail
CONTRACT: EMPAM.sol
First of all, we need to comprehensively explain the settle function, its necessary conditions and logic.
The settle function can be invoked by any caller, with the condition check that all bids must be decrypted. This means, the Queue must already include all bids in a sorted order, starting from the highest price as element 1 in the Queue (ignored element 0 as this is for initialization).
[Actually this will just be the case once the delMax() function is invoked, but that detail is not important now]
The settle function then invokes the _getLotMarginalPrice function, which determines the marginalPrice, starting from the highest price. This function follows a sophisticated pattern with many different conditions and scenarios. For the understanding of our issue it is only necessary to know that the loop breaks once the current capacity is expended and with each loop where the capacity is not expended or the price is not below the minimum price, the next bid from the Queue is fetched. Whenever the next ID is fetched, this ID is removed from the Queue and the Queue is sorted back, starting with the highest price on the index = 1.
So what does that mean? Well, if the loop breaks early, the Queue will not be fully consumed and there will be leftover bids in the Queue. The interesting part is now within the _settle function, after the marginalPrice has been determined and the Queue has been manipulated. A loop is executed over the remaining bids in the queue, which removes all properties from each specific index and completely clears all indexes within the loop. This is done until the loop is completely cleared.
The problem is that different storage operations are executed within this loop and they are gas-consuming (by now everyone knows the risk of unbounded loops and you will eventually receive many duplicates for the “unbounded loop” issue).
Consider the following scenario which emphasizes this issue:
There are hundreds or thousands of bids during the normal business logic, this is no problem because most parts of the architecture are developed in such a manner that loops can be batched.
The problem is now if there are thousands of bids and the bid with the highest price has such a large input amount that the full capacity is consumed. This will exactly trigger the previously explained mechanism within the _getLotMarginalPrice function.
After the first bid, the totalAmountIn is increased by the provided amountIn from the bid:
https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/auctions/EMPAM.sol#L680
As already mentioned, the provided price is very high (the user accepts a very low output amount for his provided input amount), which means that the capacity is already expended by the very first bid.
So that will result in the array only being decreased by 1, which basically means that the remainingBids become very large and executes a loop hundreds or thousands of times:
(https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/auctions/EMPAM.sol#L771)
This will revert due to an “out of gas” error, effectively preventing any settlement and DoS’ing the contract.
It is clear that this issue will not only happen during this very specific case. However, in my opinion this case is a very trivial prime example to prove the gas-consuming logic without fully including the heap idea in the PoC.
Impact
IMPACT:
a) DoS of settle b) Permanently locked funds
Code Snippet
https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/auctions/EMPAM.sol#L680 https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/auctions/EMPAM.sol#L771
Tool used
Manual Review
Recommendation
It might actually be a very trivial solution to ignore the remaining bids, as they are only allocated to the very specific lotId, which is never used again. Therefore, thus far i could not determine any negative side-effect on removing this (Though, still more time should be allocated to that thought-process)
In my opinion, once that is fixed, the heap mechanism will not require a change, because it will probably not run out of gas, because the capacity is most likely reached before the whole loop is consumed.