sherlock-admin4 commented 3 months ago

nirohgo

Medium

The LockStakeClipper Take function prices the Dutch auction incorrectly based on a stale market price

Summary

The LockStakeClipper dutch auction mechanism incorrectly calculates the Take() price based on the price at the time the auction started rather than an updated oracle price. This distorts the Dutch auction logic (since liquidator profitability depends on the auction price diff from the current market price) leading to either bad debt or loss for liquidated URN owners as explained below.

Vulnerability Detail

The LockStakeClipper contract manages a Dutch auction, where collateral is offered for sale at a high starting price ('buf' percent above the market price) and gradually drops the price over time (using a formula defined in the AabacusLike calc contract) until the price makes the trade profitable enough for a liquidator to take the offer.

The price is initialized in the kick() function (at the start of the auction) to the oracle market price plus the buf ratio.

sales[id].tic = uint96(block.timestamp);

uint256 top;
top = rmul(getFeedPrice(), buf);
require(top > 0, "LockstakeClipper/zero-top-price");
sales[id].top = top;

When liquidators want to buy some of the collateral they call the Take() function. Inside take, the price the liquidator gets is given by the status function. This function passes the start price and elapsed time to the calc contract (AbacusLike) that applies a time-dependant price reduction to the price. Currently there are two implementations for the price reduction: linear price drop to zero over a given timeframe (linearDecrease), or an exponential drop every given number of seconds (StairstepExponentialDecrease).

function status(uint96 tic, uint256 top) internal view returns (bool done, uint256 price) {
    price = calc.price(top, block.timestamp - tic);
    done  = (block.timestamp - tic > tail || rdiv(price, top) < cusp);
}

The core of the issue is that the price calculation in Take() doesn't account for market price changes since the auction started. For a Take() call at time t it uses the formula
$Price_t = buf OraclePriceAtAuctionStart dutchAuctionDropRatio$
instead of
$Price_t = buf OraclePriceAtTime_t dutchAuctionDropRatio$

For a liquidator, the profitability of a Take() is determined by
$liquidatorProfit = (marketPrice-acutionPrice) * mkrAmount$
Therefore for them the auction price changes only matter in relation to the market price.

Numeric example

Starting state

Mkr price: 2300 Dai
Clipper buf: 1.1 (10%)
Clipper calc formula: 0.03% linear price drop per second.

Scenario 1 - market price goes down

An auction starts with price 2300 1.1 = 2530 Dai/Mkr. Liquidator profit for 1Mkr: (2300-2530) 1 = -230
During the first 1000 secs the Mkr market price gradually drops 13% to 2001.
A liquidator calls Take after 1000 seconds. Auction price = 2530 (1-0.0003 1000) = 1771. market price = 2001. Liquidator profit for 1Mkr: (1771-2001) * 1 = -230
Because of the market price change the liquidator is still not profitable inspite of the Dutch auction dropping the price by 30%. This increases the chance that the URN enters bad debt status.

Scenario 2 - market price goes up suddenly

An auction starts with price 2300 1.1 = 2530 Dai/Mkr. Liquidator profit for 1Mkr: (2300-2530) 1 = - 230
During the first 24 secs (2 blocks) the Mkr market price rises sharply by 10% to 2530.
A liquidator calls Take after 24 seconds. Auction price = 2530 (1-0.0003 24) = 2512. market price = 2530. Liquidator profit for 1Mkr: (2530-2512) * 1 = 18
Because of the market price change, liquidation profitability rises much faster than the Dutch auction intended, causing loss for the URN owner through having their collateral sold for a worse (higher) price than they could have obtained with a real Dutch auction.

Likelihood

Since liquidations typically happen in times of high price volatility, the likelihood of sharp price movements during an auction is high, increasing the risk of distorted Dutch auction pricing (see impact below)

Root Cause

Using a stale Mkr oracle price when calculating a take() price, instead of fetching an updated price.

Impact

In scenario 1 - loss to Vat users in the form of bad debt (accumulated over time can reach 5% of assets or more)
In scenario 2 - loss to liquidated URN owners. In times of high price volatility, the price for which the URN collateral is sold can easily be lower by 10% or more compared to the attainable price with accurate Dutch auction pricing, causing a loss of over 10% to the liquidated party.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/dba30d7a676c20dfed3bda8c52fd6702e2e85bb1/lockstake/src/LockstakeClipper.sol#L348 https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/dba30d7a676c20dfed3bda8c52fd6702e2e85bb1/lockstake/src/LockstakeClipper.sol#L464

Tool used

Manual Review

Recommendation

Inside the Take function, apply buf to the current oracle price and pass that to the calc.price funcion instead of using the top value from the auction start:
$Price_t = buf OraclePriceAtTime_t dutchAuctionDropRatio$

telome commented 3 months ago

This is a design decision in Maker's liquidation system. This code is unchanged from the original clippers, so this is out of scope.

nirohgo commented 3 months ago

Escalate

This is a design decision in Maker's liquidation system. This code is unchanged from the original clippers, so this is out of scope.

A. The clipper contract is within scope and there's no mention of only changes from a previous version being in scope (not on contest readme, nor in Sherlock judging rules).

B. Sherlock's judging criteria document states this about design decisions:

"Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational."

In this case the finding clearly implies loss of funds, and therefore doesn't doesn't fall under sherlock's definition of a design decision. This contest's custom severity definitions focus on the rate of lost funds (which was demonstrated in this finding) and don't mentions any specific override of sherlock's standards on design decisions, and therefore the custom severity definitions are met as well. Further more, there is no upside or positive tradeoff to using stale prices in liquidations which would imply a design decision (nor is there any mention in the protocol's design docs that explicitly states the use of a stale price as a design decision). Therefore I argue that this finding doesn't fall under the definition of a design decision and should be a valid medium.

sherlock-admin3 commented 3 months ago

Escalate

This is a design decision in Maker's liquidation system. This code is unchanged from the original clippers, so this is out of scope.

A. The clipper contract is within scope and there's no mention of only changes from a previous version being in scope (not on contest readme, nor in Sherlock judging rules).

B. Sherlock's judging criteria document states this about design decisions:

"Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational."

In this case the finding clearly implies loss of funds, and therefore doesn't doesn't fall under sherlock's definition of a design decision. This contest's custom severity definitions focus on the rate of lost funds (which was demonstrated in this finding) and don't mentions any specific override of sherlock's standards on design decisions, and therefore the custom severity definitions are met as well. Further more, there is no upside or positive tradeoff to using stale prices in liquidations which would imply a design decision (nor is there any mention in the protocol's design docs that explicitly states the use of a stale price as a design decision). Therefore I argue that this finding doesn't fall under the definition of a design decision and should be a valid medium.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

WangSecurity commented 2 months ago

[Here's](Anything that could be considered a bug and is existing in the original Clipper is not valid) the document that was created in collaboration with Maker and Sherlock clarifying the rules for this contest (it's also linked in the contest README). It has the same weight as contest README and it clearly says:

Anything that could be considered a bug and is existing in the original Clipper is not valid

Hence, this issue is invalid, since it exists in the original Clipper contract as well.

nirohgo commented 2 months ago

OK, missed that.