search

sherlock-audit / 2024-02-perennial-v2-3-judging

6 stars 5 forks source link

thisvishalsingh - Restrictive Oracle-Market Mapping in `MarketFactory` Leading to Limited Market Creation and Impaired Revenue Potential #27

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 5 months ago

thisvishalsingh

medium

Restrictive Oracle-Market Mapping in MarketFactory Leading to Limited Market Creation and Impaired Revenue Potential

Summary

In the MarketFactory contract, a single oracle is constrained to one market due to the use of a constant key, address(0), within its market registration mapping. This design choice inadvertently limits the protocol's capacity to launch multiple markets relying on the same data source. While this does not lead to an immediate loss of funds, it causing potential revenue loss under specific conditions—the protocol's expansion and diversification efforts. Over time, this limitation could have a material impact on the protocol's financial performance and its ability to remain competitive.

mapping(IOracleProvider => mapping(address => IMarket)) private _markets;
...
_markets[definition.oracle][address(0)] = newMarket;

Vulnerability Detail

The MarketFactory contract exhibits a design vulnerability due to its mapping structure that enforces a one-to-one relationship between oracle providers and market contracts. The mapping is defined as follows:

mapping(IOracleProvider => mapping(address => IMarket)) private _markets;

Within this mapping, each IOracleProvider is intended to be linked to a corresponding IMarket. However, the contract uses a fixed key (address(0)) to represent the market for a given oracle in the inner mapping, as shown in the market creation logic:

_markets[definition.oracle][address(0)] = newMarket;

By utilizing address(0) as a universal key, the contract is limited to recognizing only one market per oracle provider. Consequently, if a new market is instantiated with an already associated oracle, it will overwrite the existing market reference in the _markets mapping.

This limitation impedes the protocol's scalability and flexibility, as it cannot support the concurrent operation of several markets that may require the same oracle data—a common scenario in decentralized finance (DeFi) platforms where oracles underpin a variety of financial instruments.

PoC

let's consider a scenario where we attempt to create two different markets that rely on the same oracle for their data feed.

  1. Market Creation Attempt 1: We deploy the first market, MarketA, and associate it with OracleX.

    IOracleProvider oracleX = IOracleProvider(0xOracleXAddress);
IMarket marketA = IMarket(0xMarketAAddress);
_markets[oracleX][address(0)] = marketA;

    After this transaction, the _markets mapping contains the association between OracleX and MarketA.

  2. Market Creation Attempt 2: We then try to create a second market, MarketB, intending to also use OracleX.

    
IMarket marketB = IMarket(0xMarketBAddress);
_markets[oracleX][address(0)] = marketB;
After executing this transaction, the mapping is updated, and MarketB is now associated with OracleX. However, MarketA is no longer referenced in the _markets mapping because the second transaction overwrote the first due to the use of the same key, address(0).

3. Result: The contract now only recognizes MarketB as being associated with OracleX, and there is no longer a reference to MarketA in the contract's state. This means that any functionality depending on the _markets mapping to interact with MarketA will fail, as the contract no longer has a record of its existence. The protocol has effectively lost the ability to interact with MarketA, and any funds or logic tied to that market are now inaccessible through the standard functions of the MarketFactory.

4.  This limitation becomes apparent when users or other contracts attempt to query or interact with MarketA through the MarketFactory. Since the mapping only holds MarketB, any calls to retrieve or perform operations on MarketA will be directed to MarketB or result in a null response.
5. To verify that MarketA has indeed been overwritten, one could query the _markets mapping with OracleX and expect to receive the address of MarketA. However, the returned address will be that of MarketB, confirming the loss of reference to MarketA.
```solidity
// Expected: 0xMarketAAddress
// Actual: 0xMarketBAddress
address marketAddress = address(_markets[oracleX][address(0)]);

PoC illustrates the vulnerability's impact on the protocol's functionality and the potential for market data to become orphaned due to the design choice of using a single static key for market registration within the MarketFactory contract.

Impact

This design constraint directly impacts the protocol's ability to expand its offerings and could lead to a significant opportunity cost. . This limitation aligns with a loss of funds under specific conditions related to the protocol's growth and market expansion strategies. It also breaks core contract functionality in terms of market diversity, which could be considered a serious non-material loss due to reduced functionality and user choice.

Code Snippet

https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/86ea166a2055f68c4a2c3ff0a2b6bef037476bb8/perennial-v2/packages/perennial/contracts/MarketFactory.sol#L79

// Mapping restricting each oracle to a single market contract
mapping(IOracleProvider => mapping(address => IMarket)) private _markets;
...
// Logic that enforces the one market per oracle constraint
_markets[definition.oracle][address(0)] = newMarket;

Tool used

Manual Review

Recommendation

sherlock-admin3 commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

panprog commented:

low, there is no real loss of funds here and this is the design choice, future growth is out of scope. Previously this map stored oracle+payoff->market links, but since payoff is now part of the oracle, the link is now just 1-to-1 (and address(0) used for compatibility with previous storage).

arjun-io commented 4 months ago

This is by design, we'd consider this invalid. As of 2.2, a market and oracle are 1 to 1