search

sherlock-audit / 2023-02-bond-judging

2 stars 0 forks source link

usmannk - Removing support for a currency pair from the oracle leaves markets in an invalid state #44

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

usmannk

medium

Removing support for a currency pair from the oracle leaves markets in an invalid state

Summary

When a new market is registered in the Bond Oracle, it is checked that that market's currencies are supported by the oracle. If not, then the transaction reverts.

However, when a currency pair is later set to be unsupported due to an issue, markets with that pair are not unregistered. This breaks the invariant that all registered markets use supported currencies.

In fact, there is actually no way for an owner to unregister a market at all.

Vulnerability Detail

Market operations on registered markets with unsupported tokens will attempts calls to address(0), causing unexpected results for oracle operations

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/BondChainlinkOracle.sol#L129-L148

Impact

Unexpected state for markets, potentially causing unexplained DoS.

Code Snippet

Tool used

Manual Review

Recommendation

Provide a mechanism for unregistering a market from the oracle. In addition, add auto-unregistering for markets using newly unsupported currency pairs.

Oighty commented 1 year ago

Owners have the ability to close markets they have active on any Auctioneer. If an oracle removes support for a token pair, then the currentPrice() function will fail. Specifically, it will execute the path _getOneFeedPrice and then _validateAndGetPrice, which will revert when validating the empty data returned from the call to the zero address.

However, this is mostly by coincidence. I think the best approach is having currentPrice() revert if the priceFeedParams for the pair are bytes(0) since this will be the case immediately after the pair is no longer supported and provides clearer behavior.

Oighty commented 1 year ago

Issue fixed here: https://github.com/Bond-Protocol/bonds/pull/52

xiaoming9090 commented 1 year ago

Fixed in https://github.com/Bond-Protocol/bonds/pull/52

hrishibhat commented 1 year ago

After revisiting this issue, it needed to be discussed internally and rejudged. Considering this issue as low based on following points:

Removing support for currency pair needs to be done by the owner calling setPair. This issue is a low as the only impact is that users would no longer be able to buy new bond tokens from the market owner because the currentPrice function would revert - no asset loss. Bond token holders (users) as they could continue to redeem their Bond tokens via the BondFixedTermTeller.redeem as per normal when their bond tokens have matured because the redeem function does not rely on the affected currentPrice function. As the Sponsor pointed out the market owners can close the market if they wish or add the token back if needed. Considering this issue as low.