0x52 - Full inventory asset purchases can be DOS'd via frontrunning

[sherlock-admin]

0x52

medium

Full inventory asset purchases can be DOS'd via frontrunning

Summary

Users who attempt to swap the entire component value can be frontrun with a very small bid making their transaction revert

Vulnerability Detail

AuctionRebalanceModuleV1.sol#L795-L796

    // Ensure that the component quantity in the bid does not exceed the available auction quantity.
    require(_componentQuantity <= bidInfo.auctionQuantity, "Bid size exceeds auction quantity");

When creating a bid, it enforces the above requirement. This prevents users from buying more than they should but it is also a source of an easy DOS attack. Assume a user is trying to buy the entire balance of a component, a malicious user can frontrun them buying only a tiny amount. Since they requested the entire balance, the call with fail. This is a useful technique if an attacker wants to DOS other buyers to pass the time and get a better price from the dutch auction.

Impact

Malicious user can DOS legitimate users attempting to purchase the entire amount of component

Code Snippet

AuctionRebalanceModuleV1.sol#L772-L836

Tool used

Manual Review

Recommendation

Allow users to specify type(uint256.max) to swap the entire available balance

[pblivin0x]

The recommendation here is that we allow componentQuantity to be specified in excess of the auction size? so replace the current check

// Ensure that the component quantity in the bid does not exceed the available auction quantity.
require(_componentQuantity <= bidInfo.auctionQuantity, "Bid size exceeds auction quantity");

with a enforced cap

if (_componentQuantity > bidInfo.auctionQuantity) {
    _componentQuantity = bidInfo.auctionQuantity;
}

I was originally hesitant because of some unintuitive UX, but if this removes the potential for a DOS attack, i think it is worth implementing.

[FlattestWhite]

Hmmm we should probably allow user to specify maxQuantity rather than the absolute quantity they want to buy

[snake-poison]

The recommendation here is that we allow componentQuantity to be specified in excess of the auction size? so replace the current check

// Ensure that the component quantity in the bid does not exceed the available auction quantity.
require(_componentQuantity <= bidInfo.auctionQuantity, "Bid size exceeds auction quantity");

with a enforced cap

if (_componentQuantity > bidInfo.auctionQuantity) {
    _componentQuantity = bidInfo.auctionQuantity;
}

I was originally hesitant because of some unintuitive UX, but if this removes the potential for a DOS attack, i think it is worth implementing.

I believe what the submitter was recommending was something more akin to keeping the :

 if (_componentQuantity == type(uint256).max) {
     _componentQuantity = bidInfo.auctionQuantity;
} else {
require(_componentQuantity <= bidInfo.auctionQuantity, "Bid size exceeds auction quantity");
}

note: the difference in gas is trivial, but it isn't equivalent to your suggestion so I wanted to point it out. My examples assumes pragma > 0.7 to use the type().max syntax but the older idiomatic uint256(-1) would.

[sherlock-admin2]

Escalate

Severity should not be Medium, has to be low or invalid, because there is no incentive at all for front-runners and also based on Sherlock's documentation, DOS < 1yr is not a valid one.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page,
in the Sherlock webapp.

[JJtheAndroid]

Escalate

This issue should be low/invalid as per Sherlock's rules https://docs.sherlock.xyz/audits/judging/judging. In addition, this is an edge case with no user funds at risk.

[sherlock-admin2]

Escalate

This issue should be low/invalid as per Sherlock's rules https://docs.sherlock.xyz/audits/judging/judging. In addition, this is an edge case with no user funds at risk.

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.

[Arabadzhiew]

Escalate

This issue should be low/invalid as per Sherlock's rules https://docs.sherlock.xyz/audits/judging/judging. In addition, this is an edge case with no user funds at risk.

Can't agree on this. This is definitely not an edge case. There is an incentive to perform such kind of a DoS - If the price of the auction decreases over time, users will probably want to prolong it as much as they can. Additionally, the DoS can very easily happen unintentionally, when a lot of bid transactions get executed in the same block.

[JJtheAndroid]

Escalate
This issue should be low/invalid as per Sherlock's rules https://docs.sherlock.xyz/audits/judging/judging. In addition, this is an edge case with no user funds at risk.

Can't agree on this. This is definitely not an edge case. There is an incentive to perform such kind of a DoS - If the price of the auction decreases over time, users will probably want to prolong it as much as they can. Additionally, the DoS can very easily happen unintentionally, when a lot of bid transactions get executed in the same block.

In your scenario, users can only prolong it as long the current price is higher than min price. Malicious front running is useless beyond that point, because the price cannot go any lower. This would be invalid as per Sherlock rules on DOS attacks.

The unintentional scenario is not a DOS, it is just multiple people bidding at the same time. This is by design.

Again, no user funds are at risk. This should not be a med

[IAm0x52]

Can't agree on this. This is definitely not an edge case. There is an incentive to perform such kind of a DoS - If the price of the auction decreases over time, users will probably want to prolong it as much as they can. Additionally, the DoS can very easily happen unintentionally, when a lot of bid transactions get executed in the same block.

Agreed with this. Given the nature of a dutch auction, the temporary DOS will prevent the auction from executing as expected and lead to assets being sold under market value.

[pblivin0x]

I agree this is a valid Medium

[auditsea9]

valid Medium? 🤔🤔🤔

[auditsea9]

Here's the docs about the severity of DOS: https://docs.sherlock.xyz/audits/judging/judging

Hope this helps in escalation! Thanks y'all! 👍

[hrishibhat]

@0xauditsea @JJtheAndroid
There seems to be some confusion about the rule for DOS. Funds not being accessible temporarily does not apply here.
Here the DOS results in loss of funds which is considered to be a medium:

Agreed with this. Given the nature of a dutch auction, the temporary DOS will prevent the auction from executing as expected and lead to assets being sold under market value.

[auditsea9]

@hrishibhat - Why is there loss of funds at all? The auction has minimum price defined which can be lower than the market value, that's totally fine for users to buy tokens with lower price, it's benefit for users, acceptable for auction manager.

[hrishibhat]

@pblivin0x @0xauditsea
Correct me if I'm wrong, assuming here it is a Dutch auction where price reduces over time. If i can DOS someone who is ready to bid for a higher price and I bid for a lesser price after some time. Isn't this an issue?

[hrishibhat]

To add to my comment:
the Max min values are values that are acceptable by the manager if the users decide to bid for the value within that range.
That does not mean the code should unfairly allow someone to stop a higher bid and bid at a lower price. This seems like valid issue that does not allow normal functioning of dutch auction.

[auditsea9]

@hrishibhat - In selling auction (users buy component), component price goes lower as time goes. So when it's front-run, the user will try again with lower price, which is fine for the user, also no problem with the auction manager.

[auditsea9]

This is basically not loss of funds.

[JJtheAndroid]

To add to my comment: the Max min values are values that are acceptable by the manager if the users decide to bid for the value within that range. That does not mean the code should unfairly allow someone to stop a higher bid and bid at a lower price. This seems like valid issue that does not allow normal functioning of dutch auction.

I don't want to go back and forth on this. I just want to say that your description is inaccurate. The report does not describe DOS on higher bids, it is a DOS on full inventory bids which is a rarer occurrence. Big difference. Also such an "attack" does not benefit the attacker nor does it hurt the "victim" because they both want to buy assets at a lower price.

Finally, there is a min price bound set by the manager. So any full inventory DOS at that point is completely useless and a waste of gas.

All of this is assuming that there are only 2 actors (the attacker and the victim) bidding. In reality, there will likely be many more and not all of them will submit a full inventory bid.

I will not comment on this further

[pblivin0x]

@pblivin0x @0xauditsea Correct me if I'm wrong, assuming here it is a Dutch auction where price reduces over time. If i can DOS someone who is ready to bid for a higher price and I bid for a lesser price after some time. Isn't this an issue?

We have a 1000 ETH auction, a legitimate bidder wants to settle the full 1000 ETH auction at PRICE_HIGH

The malicious bidder is willing to settle the 1000 ETH auction at PRICE_LOW, so they frontrun the legitimate bidder with SMALL_SIZE bid.

If some transactions are successfully DOS'd, the legitimate bidder could submit a 1000 ETH - SMALL_SIZE bid, or something like a 500 ETH bid, such that the malicious bidder is not willing to front run at that size

[Oot2k]

Escalate
This issue should be low/invalid as per Sherlock's rules https://docs.sherlock.xyz/audits/judging/judging. In addition, this is an edge case with no user funds at risk.

Can't agree on this. This is definitely not an edge case. There is an incentive to perform such kind of a DoS - If the price of the auction decreases over time, users will probably want to prolong it as much as they can. Additionally, the DoS can very easily happen unintentionally, when a lot of bid transactions get executed in the same block.

I think this is valid, this comment explains it well.

[auditsea9]

@pblivin0x @0xauditsea Correct me if I'm wrong, assuming here it is a Dutch auction where price reduces over time. If i can DOS someone who is ready to bid for a higher price and I bid for a lesser price after some time. Isn't this an issue?

We have a 1000 ETH auction, a legitimate bidder wants to settle the full 1000 ETH auction at PRICE_HIGH

The malicious bidder is willing to settle the 1000 ETH auction at PRICE_LOW, so they frontrun the legitimate bidder with SMALL_SIZE bid.

If some transactions are successfully DOS'd, the legitimate bidder could submit a 1000 ETH - SMALL_SIZE bid, or something like a 500 ETH bid, such that the malicious bidder is not willing to front run at that size

Totally agree with this, no way front-runners would try it.

[Arabadzhiew]

@pblivin0x @0xauditsea Correct me if I'm wrong, assuming here it is a Dutch auction where price reduces over time. If i can DOS someone who is ready to bid for a higher price and I bid for a lesser price after some time. Isn't this an issue?

We have a 1000 ETH auction, a legitimate bidder wants to settle the full 1000 ETH auction at PRICE_HIGH

The malicious bidder is willing to settle the 1000 ETH auction at PRICE_LOW, so they frontrun the legitimate bidder with SMALL_SIZE bid.

If some transactions are successfully DOS'd, the legitimate bidder could submit a 1000 ETH - SMALL_SIZE bid, or something like a 500 ETH bid, such that the malicious bidder is not willing to front run at that size

Correct me if I'm wrong, but even in your example, the legitimate bidder is most likely still going to end up buying the ETH at a lower price, leading to the protocol receiving less assets that it could have received.

The main issue here is that due to the current implementation, full inventory purchases are going to end up being reverted most of the time, be it due to intentional DoS attacks, or simply because there were a lot of bid transactions executed at the given time (for example, if there were 5 bid transactions sitting in the mempool with the same gas price and one of them was for a full inventory purchase, if that one does not get executed first, it will simply be reverted), leading to the component assets being sold at lower prices.

[auditsea9]

@Arabadzhiew
You guys keep saying components are being sold at lower prices, if auction manager doesn't want them to be sold at lower prices, they should increase MIN price. When auction manager defines MIN price, it surely means that purchase at MIN price is pretty acceptable, this is pretty logical thing. Components being sold at lower price, good for buyers, acceptable for the auction manager, what is the problem here at all?

Regarding the example you mentioned above, you are right that full purchase bid will be reverted when there is another bid tx is executed before it, I think that's fine, that's what the auction is for - first buyer gets what they want.
If you don't agree with this and let full purchase tx buy all remaining amount, it will cause an issue like, users wanted to buy whole 10WETH from the auction but they end up only buying 5WETH, do you think users will like this?

Ofc this issue needs to be fixed, but the severity can not be Med at all.

[Arabadzhiew]

@Arabadzhiew You guys keep saying components are being sold at lower prices, if auction manager doesn't want them to be sold at lower prices, they should increase MIN price. When auction manager defines MIN price, it surely means that purchase at MIN price is pretty acceptable, this is pretty logical thing. Components being sold at lower price, good for buyers, acceptable for the auction manager, what is the problem here at all?

Regarding the example you mentioned above, you are right that full purchase bid will be reverted when there is another bid tx is executed before it, I think that's fine, that's what the auction is for - first buyer gets what they want. If you don't agree with this and let full purchase tx buy all remaining amount, it will cause an issue like, users wanted to buy whole 10WETH from the auction but they end up only buying 5WETH, do you think users will like this?

Ofc this issue needs to be fixed, but the severity can not be Med at all.

Sure, the MIN value is defined by the auction manager, but the fact that the component asset is going to be sold at a lower price is still a loss of funds for the protocol. The MIN value is there to make sure that the auction targets get reached, but I don't think the protocol team should be ok with receiving less assets, when they can receive more.

Also, regarding the mitigation recommended in this report, I think it is fine due to the fact that it is optional - users can only use the entire available balance purchase functionality if they explicitly say so, otherwise the bidding functionality should work as it currently does.

I won't comment on this issue any further. Let's let the Sherlock team decide whether it is a valid medium or not.

[hrishibhat]

Result:
Medium
Has duplicates
After consideration of the above comments this issue is a valid medium, DOS of a valid bid at a certain price in a Dutch auction is considered damage to how the auction functions.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• JJtheAndroid: rejected

[hrishibhat]

Additionally, I see that the Sherlock rules are being interpreted incorrectly, will make sure to make the necessary changes to the docs and see how best any possible confusion can be avoided.

[pblivin0x]

The remediation for this issue is open for review here IndexCoop/index-protocol#25

The changes are to allow users to specify type(uint256.max) to settle the remaining auction https://github.com/IndexCoop/index-protocol/blob/839a6c699cc9217c8ee9f3b67418c64e80f0e10d/contracts/protocol/modules/v1/AuctionRebalanceModuleV1.sol#L811-L817

[IAm0x52]

Fix looks good. User can now set _componentQuantity to type(uint256.max) to buy the entire amount remaining