search

sherlock-audit / 2022-11-float-capital-judging

2 stars 1 forks source link

dic0de - Market Can Be Deprecated more than once #37

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

dic0de

high

Market Can Be Deprecated more than once

Summary

The MarketCore.sol contract has a deprecate mechanism as a fall back mechanism in case of a black swan event. The call to deprecate the market can be done through the two following means:

  1. If there is no market updates after 10 days then anyone can call deprecate as shown in the deprecateMarketNoOracleUpdates () https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L625-L630.
  2. If the admin initiates the deprecate process as shown here https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L632-L635.

Both means to deprecate the market further calls the internal _deprecateMarket () function which updates the epochInfo.latestExecutedEpochIndex as shown here https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L602-L621.

The changed epochInfo.latestExecutedEpochIndex is used in the calculation of the amount when users are exiting the deprecated market as shown uint256 amount = balance.mul(poolToken_priceSnapshot[epochInfo.latestExecutedEpochIndex][poolType][poolIndex]); in the _exitDeprecatedMarket () function as shown here: https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L639-L662

As such, repetitive invocation of the _deprecateMarket () function will inflate the epochInfo.latestExecutedEpochIndex by 2 as shown epochInfo.latestExecutedEpochIndex += 2; which will affect the users' withdrawn amounts when they exit the deprecated market.

Vulnerability Detail

The means to deprecate the market calls the internal _deprecateMarket () function which updates the epochInfo.latestExecutedEpochIndex as shown here https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L602-L621.

The changed epochInfo.latestExecutedEpochIndex is used in the calculation of the amount when users are exiting the deprecated market as shown uint256 amount = balance.mul(poolToken_priceSnapshot[epochInfo.latestExecutedEpochIndex][poolType][poolIndex]); in the _exitDeprecatedMarket () function as shown here: https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L639-L662

Impact

Repetitive invocation of the _deprecateMarket () function will inflate the epochInfo.latestExecutedEpochIndex by 2 which will affect the users' withdrawn amounts when they exit the deprecated market.

Code Snippet

  1. https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L625-L630.
  2. https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L632-L635.
  3. https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L602-L621
  4. https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L639-L662

Tool used

Manual Review

Recommendation

Consider ensuring deprecateMarket () cannot be invoked more than once.

WooSungD commented 1 year ago

As such, repetitive invocation of the _deprecateMarket() function will inflate the epochInfo.latestExecutedEpochIndex by 2 as shown epochInfo.latestExecutedEpochIndex += 2; which will affect the users' withdrawn amounts when they exit the deprecated market.

It is true that epochInfo.latestExecutedEpochIndex will be incremented by 2 every time _deprecateMarket() is called. However, this has NO impact on the amounts that users can withdraw, because the pool token price remains the same.

When _deprecateMarket() is called, _rebalancePoolsAndExecuteBatchedActions() is called with 0 price change and emptyValueChangeAndFunding. https://github.com/sherlock-audit/2022-11-float-capital/blob/090c1096aacc0e7dc31bc1d00a82357f9c76fbd4/contracts/market/template/MarketCore.sol#L609

Therefore the liquidities do not shift between pools when it is called, thus not resulting in shift in pool values when _deprecateMarket()is called multiple times, thus not resulting in any change in pool token price.

Even if some users have redeemed their tokens from a deprecated market, the reduction in pool value will have been done at the pool token price at deprecation, which would not affect pool token price for subsequent redeems (ratio of pool value to number of tokens will remain the same).

Adding a check to prevent deprecating a market that has already been deprecated is sensible - will discuss with team.

JasoonS commented 1 year ago

I modified one of the tests to verify that these seems to be nothing to gained by an attacker from calling this multiple times: image

Have added the check regardless.

Evert0x commented 1 year ago

Downgrading to low as there is no material loss and can only be triggered by admin

JasoonS commented 1 year ago

https://github.com/Float-Capital/monorepo/pull/3819 - we added this for prudence. As the test shows no known issue with calling this function multiple times.