sherlock-audit / 2023-06-Index-judging

6 stars 2 forks source link

ast3ros - Malicious users can exploit the auction and make profit when the SetToken is not locked. #57

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

ast3ros

high

Malicious users can exploit the auction and make profit when the SetToken is not locked.

Summary

The SetToken can be minted and redeemed by anyone when it is not locked during rebalancing. This can allow malicious users to front-run and back-run the bidders and manipulate the auction outcome.

Vulnerability Detail

When rebalancing, the token manager can configure if the SetToken is locked or not. If the SetToken is not locked, anyone can mint and redeem the SetToken using BasicIssuanceModule. The token manager can also configure the pricing mechanism via the priceAdapter. There are some mechanisms:

Let's see an example:

The current price of WETH is 1940 USDC. Total supply of the SetToken is 10.

A Set Token with component WETH and current unit(1 WETH) wants to achieve target unit (0.5 WETH - 975 USDC).

To achieve this, it needs to sell WETH to buy USDC. The manager starts rebalancing using linear price curve: start at $2000, lower to minimum $1900, take steps of $0.1 every minute. It also chooses USDC as the quote token.

Assuming when the price of WETH reaches 1950 USDC, a bidder bids for all of the available WETH for the rebalance process, which is 0.5 WETH per Set Token or 5 WETH in total for 9750 USDC (5*1950). The expected result should be that the SetToken will meet the target and the rebalancing process will finish. The end position will be:

However, the module is deployed on mainnet and polygon, a malicious user can front-run the bidder and mint the SetToken to make profit and disrupt the auction. The malicious user mints 10 SetToken using 10 WETH. It increases the total supply of the SetToken to 20.

After that, the malicious user can back-run the bidding transaction and redeem his 10 SetToken for 7.5 ETH and 4875 USDC. Malicious user balance:

The malicious user can make a profit of 25 USDC and disrupt the auction because the auction cannot finish as it should be.

He cannot make a profit directly by bidding because the bidder may need to be whitelisted by the manager.

In conclusion, if the price of auction is above the market price and a bid is placed, a malicious user can front-run and back-run the bidder and make a profit and disrupt the auction in the unlocked rebalancing process.

Impact

The malicious user can make a profit and prevent the auction from meeting the target and finishing.

Code Snippet

https://github.com/sherlock-audit/2023-06-Index/blob/main/index-protocol/contracts/protocol/modules/v1/AuctionRebalanceModuleV1.sol#L254-L257

Tool used

Manual Review

Recommendation

The SetToken should be always locked when rebalancing.

thangtranth commented 1 year ago

Escalate

This is not the duplication of #21 since it does not require ERC777. Please help to review.

sherlock-admin2 commented 1 year ago

Escalate

This is not the duplication of #21 since it does not require ERC777. Please help to review.

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.

pblivin0x commented 1 year ago

In conclusion, if the price of auction is above the market price and a bid is placed, a malicious user can front-run and back-run the bidder and make a profit and disrupt the auction in the unlocked rebalancing process.

If the bidder is bidding on an auction that is above market price, then yes, there is profit to be made in the system. That is by design.

An example of this would be a bidder desperate to exit a position, and willing to incur slippage to exit, just so happens that the exit is market making the SetToken auction.

Invalid issue, note that we've added logic to settle remaining units as part of remediation for #41

hrishibhat commented 1 year ago

@thangtranth

pblivin0x commented 1 year ago

For the security purposes of this audit, it can be considered that the price set by the trusted SetToken manager is valid, and any NAV decay of the SetToken needs to be measured against these prices.

By allowing bid's, mints, and redeems there is no clear way to decay NAV as defined by the auction prices.

Any arbitrage open between the auction prices and market prices is to be handled by the SetToken manager.

thangtranth commented 1 year ago

Hi @pblivin0x,

If the bidder is bidding on an auction that is above market price, then yes, there is profit to be made in the system. That is by design.

In this issue, it shows that the profit goes to the malicious user who is not the current SetToken holder. He only buys SetToken when there is a profit bidding auction and sells immediately after that by back running.

He can make risk free money and can prevent the auction from getting the target. In the example above, if the bidder bids 9750 USDC, the auction should be completed, however it is not because profit is extracted by malicious user. Actually a MEV can increase the buying and selling amount and extract most of the profit from the bidding.

Because the protocol is deployed in mainnet and polygon then it is very common to happen. Therefore it should be addressed.

An example of this would be a bidder desperate to exit a position, and willing to incur slippage to exit, just so happens that the exit is market making the SetToken auction.

There is nothing wrong with the bidder here. He gets his expected bidding price for the assets.

pblivin0x commented 1 year ago

Hi @pblivin0x,

If the bidder is bidding on an auction that is above market price, then yes, there is profit to be made in the system. That is by design.

In this issue, it shows that the profit goes to the malicious user who is not the current SetToken holder. He only buys SetToken when there is a profit bidding auction and sells immediately after that by back running.

He can make risk free money and can prevent the auction from getting the target. In the example above, if the bidder bids 9750 USDC, the auction should be completed, however it is not because profit is extracted by malicious user. Actually a MEV can increase the buying and selling amount and extract most of the profit from the bidding.

Because the protocol is deployed in mainnet and polygon then it is very common to happen. Therefore it should be addressed.

An example of this would be a bidder desperate to exit a position, and willing to incur slippage to exit, just so happens that the exit is market making the SetToken auction.

There is nothing wrong with the bidder here. He gets his expected bidding price for the assets.

My current understanding is this, I would love to get more opinions.


If:

Then:

Fix:


If there is no supply cap on an unlocked auction, then anytime a malicious actor sees a bid() at a price sufficiently higher than their perceived price, then the malicious actor can make risk free money and can prevent the auction from getting the target. - @thangtranth

FlattestWhite commented 1 year ago
After that, the malicious user can back-run the bidding transaction and redeem his 10 SetToken for 7.5 ETH and 4875 USDC. Malicious user balance:

Before: 10 WETH = 19400 USDC
After: 7.5 WETH + 4875 USDC = 7.5 * 1940 + 4875 = 19425 USDC.

Why is Before 10 WETH = 19400 USDC? Isn't it 10 WETH = 19500 USDC After: 7.5 WETH + 4875 USDC = 7.5 * 1950 + 4875 = 19500 USDC

pblivin0x commented 1 year ago

Would a mint/redeem fee prevent the sandwich attack? @thangtranth

thangtranth commented 1 year ago

Would a mint/redeem fee prevent the sandwich attack? @thangtranth

I think it makes the attack more expensive. Then the attacker needs to consider the amount of fees that he has to pay for mint + redeem and the profit gained (the gap between bid price and current price). If we config the fees large enough then it may. However honest users will have to pay the fee as well.

thangtranth commented 1 year ago
After that, the malicious user can back-run the bidding transaction and redeem his 10 SetToken for 7.5 ETH and 4875 USDC. Malicious user balance:

Before: 10 WETH = 19400 USDC
After: 7.5 WETH + 4875 USDC = 7.5 * 1940 + 4875 = 19425 USDC.

Why is Before 10 WETH = 19400 USDC? Isn't it 10 WETH = 19500 USDC After: 7.5 WETH + 4875 USDC = 7.5 * 1950 + 4875 = 19500 USDC

Please refer to the scenario: The current price of WETH is 1940 USDC. The 1950 is the price of the bid from bidder.

0xauditsea commented 1 year ago

I don't think this is valid, when those kind of MEV is allowed, auction managers will allow tokens not being locked, but otherwise _shouldLockSetToken will be set to true.

thangtranth commented 1 year ago

I don't think this is valid, when those kind of MEV is allowed, auction managers will allow tokens not being locked, but otherwise _shouldLockSetToken will be set to true.

Hi, it is already confirmed with the protocol team that this is not the intended behaviour when MEV is allowed when unlocked. From the protocol team:

it is concerning...it is ideal for SetToken's to actually not be locked during rebalancing bc we want users to always have access to their underlying

bizzyvinci commented 1 year ago

The issue with this issue is that it assumes a logical bidder would trade at a loss against SetToken.

I'll start with some axioms which I believe are True

If the axioms listed above are True. Then the following should be True

P.S: Another reason I believe SetToken is comfortable at trading at discount loss is because Auction could be used against DEX slippage or as an order book that would be executed at a future time when price reaches the limit set by manager.

And

P.S: quote asset must be a component for second point to be True. And I believe that's done because the contract checks that bidded component is not quoteAsset to avoid attacks when quoteAsset is part of SetToken components. And most importantly, it updates both quoteAsset and bidded component position here.

Therefore:

Using numbers would be more complicated cause there are several parameters. And the proof provided by the issue is flawed because SetToken is selling component at a price higher than market/DEX. This means that bidder is taking a loss and SetToken is taking a profit. That's the source of the $25. Proof:

If the bidder is logical he would wait till price is below market price so that he'll make a profit on the trade. Therefore sandwicher would take part in the loss.

sinarette commented 1 year ago

Should not be able to decay the SetToken Net-Asset-Value according to this price. with any combination of actions (bids, mints, or redeems)

According to the contest readme it requires damage calculated according to the bid price. Here the stated attack scenario is just buying ETH at 1940 USDC and selling at 1950 USDC; we don't tell this kind of profitable trading stategy an 'attack'. In fact, in ETH units it's not a profit; the attacker who had 19400/1940 = 10 ETH now has 19425/1950 = 9.96 ETH.

pblivin0x commented 1 year ago

Agree with @bizzyvinci here that bidder's in the system are expected to be rationale profit seeking actors - If the bidder is logical he would wait till price is below market price so that he'll make a profit on the trade. Therefore sandwicher would take part in the loss.

thangtranth commented 1 year ago

Yes, I also agree with @bizzyvinci . A very good point about rationale bidder 👍

pblivin0x commented 1 year ago

Thank you for all the input here @thangtranth @bizzyvinci @0xauditsea @sinarette @Oot2k

After much consideration I'm of the following opinion

Index is prepared to take the following remediations

I believe this is a Medium severity issue because

Oot2k commented 1 year ago

Agree with escalation and agree that this is valid.

hrishibhat commented 1 year ago

Result: Medium Unique Considering this a medium issue based on above comments https://github.com/sherlock-audit/2023-06-Index-judging/issues/57#issuecomment-1666224199

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

pblivin0x commented 1 year ago

The remediation for this issue is open for review here https://github.com/IndexCoop/index-protocol/pull/25

The changes are to add warnings to use a supply cap in order to avoid large front running issuance and redemption

IAm0x52 commented 1 year ago

Sponsor has not made any changes to the smart contract but acknowledges this scenario and expects manager (trusted party) to set appropriate supply caps to prevent this.

MLON33 commented 1 year ago

Confirming no changes to the smart contract. Classified as acknowledged instead of fixed.