Factory Lacks a Key to enable intuitive / easy Market tracking

[hats-bug-reporter[bot]]

Github username: @@kodakr Twitter username: @Kodak_Rome Submission hash (on-chain): 0x4031af51a0dd99f6648c78a3a3065eccebd7fac0c7c6b0703e79cabbb76c74ff Severity: medium

Description: Description\ The MarketFactory contract is designed to deploy multiple instances of market contracts, and it provides functions such as allMarkets() and marketCount() to query the created markets. However, upon analysis, I found that these implementations can only return abiguous and non specific details. They alone fall short in terms of usability and practicality, especially as the number of markets increases.

Lets take a scenario at 3,000 deployments.

The issue arises when users or creators attempt to retrieve a specific market from the list of all deployed markets. Both allMarkets() and marketCount() return non-specific and ambiguous data. For instance:

• allMarkets() returns an array of addresses (address[]), which, when dealing with a large number of markets (e.g., 3000), makes it impractical for users to retrieve a specific market.
• marketCount() simply returns a uint, which indicates the number of deployed markets but does not help in identifying a specific market.

This is inefficient and unintuitive for users, especially if they are trying to locate a specific market based on an index or other identifiers. Simply providing a large array of addresses doesn’t help the user easily identify the market of interest.

Illustration of the Issue\

Consider the scenario where 3000 market contracts have been deployed by the MarketFactory. If a user wants to retrieve the address of a specific market (e.g., the 1500th market), they would have to either:

Iterate over the entire list of 3000 addresses returned by allMarkets(). Manually identify the specific market by scanning through the list, which is highly inefficient and prone to errors, as contract addresses are not human-readable. This method of querying is cumbersome and unsuitable for a user-friendly dApp or contract interface.

Attachments

Proposed Mitigation

// Mapping that stores a unique index and its corresponding market address
mapping(uint => address) public uniqueIdToMarketMap;

// New function to retrieve the market address by index
function getSpecificMarketByIndex(uint index) public view returns(address) {
    return uniqueIdToMarketMap[index];
}

---

Revised Code File (Optional)

$ git diff
diff --git a/Market.sol b/Market.sol
index 9b2462b..010513f 100644
--- a/Market.sol
+++ b/Market.sol
@@ -23,7 +23,7 @@ contract Market {
     struct RealityParams {
         bytes32[] questionsIds;
         uint256 templateId;
-        string[] encodedQuestions;
+        string[] encodedQuestions; // @audit how does size affect this
     }

     /// @dev Contains the information associated to Conditional Tokens.
@@ -58,6 +58,9 @@ contract Market {
     RealityParams public realityParams;
     /// @dev Oracle contract.
     RealityProxy public realityProxy;
+    /// @dev Unique FactoryId
+    uint public uniqueFactoryId;
+

     /// @dev Initializer.
     /// @param _marketName The name of the market.
@@ -74,7 +77,8 @@ contract Market {
         uint256 _upperBound,
         ConditionalTokensParams memory _conditionalTokensParams,
         RealityParams memory _realityParams,
-        RealityProxy _realityProxy
+        RealityProxy _realityProxy,
+        uint _uniqueFactoryId
     ) external {
         require(!initialized, "Already initialized.");

@@ -85,6 +89,7 @@ contract Market {
         conditionalTokensParams = _conditionalTokensParams;
         realityParams = _realityParams;
         realityProxy = _realityProxy;
+        uniqueFactoryId = _uniqueFactoryId;

         initialized = true;
     }
@@ -164,7 +169,7 @@ contract Market {
     }

     /// @dev Helper function to resolve the market.
-    function resolve() external {
+    function resolve() external { //@audit anyone can call resolve??
         realityProxy.resolve(this);
     }
-}
\ No newline at end of file
+}
diff --git a/MarketFactory.sol b/MarketFactory.sol
index fc952b6..c2d6827 100644
--- a/MarketFactory.sol
+++ b/MarketFactory.sol
@@ -82,7 +82,13 @@ contract MarketFactory {
     /// @dev Oracle contract.
     RealityProxy public immutable realityProxy;
     /// @dev Markets created by this factory.
-    address[] public markets;
+    //@audit what happens at 256??
+    address[] public markets; 
+    /// @dev unique factory deployment Identification(FDI) number. increments on each deploy.
+    uint private uniqueFactoryId;
+    // Mapping that stores a unique index and its corresponding market address
+    mapping(uint uniqueFactoryId => address)uniqueIdToMarketMap;
+    
     /// @dev Market contract.
     address public immutable market;

@@ -234,11 +240,13 @@ contract MarketFactory {
     /// @param marketName The market name.
     /// @param config InternalMarketConfig instance.
     /// @return The new market address.
+    
     function createMarket(
         CreateMarketParams memory params,
         string memory marketName,
         InternalMarketConfig memory config
     ) internal returns (address) {
+        //@audit market without an outcome can be created.
         (Market.ConditionalTokensParams memory conditionalTokensParams, Market.RealityParams memory realityParams) =
             createNewMarketParams(params, config);

@@ -251,7 +259,8 @@ contract MarketFactory {
             params.upperBound,
             conditionalTokensParams,
             realityParams,
-            realityProxy
+            realityProxy,
+            uniqueFactoryId 
         );

         emit NewMarket(
@@ -264,6 +273,8 @@ contract MarketFactory {
         );

         markets.push(address(instance));
+        uniqueIdToMarketMap[uniqueFactoryId++] = address(instance); //increments and appends
+        

         return address(instance);
     }
@@ -379,7 +390,7 @@ contract MarketFactory {
             )
         );

-        if (realitio.getTimeout(question_id) != 0) {
+        if (realitio.getTimeout(question_id) != 0) { //@audit 
             return question_id;
         }

@@ -471,4 +482,12 @@ contract MarketFactory {
     function marketCount() external view returns (uint256) {
         return markets.length;
     }
-}
\ No newline at end of file
+
+    // New function to retrieve the market address by index
+    function getSpecificMarketByIndex(uint index) public view returns(address){
+        return uniqueIdToMarketMap[index];
+
+    }
+
+    
+}
(END)

Conclusion:

This can atually cause a temporal DOS in a long term based Reality question to a user. Hence flagged Medium severity. The current implementation of allMarkets() and marketCount() does not provide an effective mechanism for users to query specific markets efficiently. The introduction of an indexed mapping (uniqueIdToMarketMap) and the getSpecificMarketByIndex() function is a necessary mitigation to address this issue. This update will enhance the user experience and improve the contract's scalability as the number of deployed markets increases.

[clesaege]

From the address, you can get the markets. Indexing and data retrieval for display is the job of the Graph, not the smart contract. Users use frontends, they don't call smart contract view functions by hand.

[kodakr]

1. Your response implies that you may have misunderstood the core concern.

   From the address, you can get the markets.

From which address? The entire issue raised was about the inefficiency in retrieving the market address at a later time (long after deployment). The problem here is how we get that address efficiently, and I don't think you've acknowledged that. I never meant at time of deploy.... of course the factory returns the address at deploy time.

2. Completely wrong!

   Indexing and data retrieval for display is the job of the Graph, not the smart contract. Users use frontends, they don't call smart contract view functions by hand.

3. Heavily biased assumptions.

   Users use frontends, they don't call smart contract view functions by hand.

It is fair to point out a bias here. Your argument assumes all users interact with protocols via front-end interfaces. This is simply not true. Most users, including myself, direct contract interaction as the primary mode of interaction. There’s an entire ecosystem of users who bypass the front end interacting directly on SC. Relying on The Graph without considering these users is shortsighted. This reason is both biased and incorrect. Even if it were accurate (it’s not), there's still limitations in user interaction and data retrieval on-chain.

4. Lack of composability Programmatic derivation What happens for a user who is scripting in Solidity or programmatically deriving data without a front end? They can't rely on The Graph to fetch already deployed Market's address onchain and retrieving market addresses directly from the contract is not just preferred but required. Why introduce unnecessary friction for them?

5. DOS on integrating Contracts Integrating protocols What about protocols trying to integrate with yours? Do they also need to rely on The Graph to retrieve market addresses and relevant data? What if they want to fetch this data on-chain, directly from your smart contract? The lack of a direct retrieval method limits composability.

6. What is the downside to adding this? Finally, what exactly are we losing by implementing this? There’s zero downside to having an efficient way to retrieve specific markets directly from the contract. The contract remains usable by both front-end interfaces and external protocols, and it gives more flexibility to a wider (all) array of users.

In conclusion, By neglecting this functionality, you're introducing friction for advanced users and protocols who don't rely on The Graph. In decentralized systems, maximizing accessibility and minimizing reliance on third-party solutions are key. This change improves efficiency and broadens usability without any notable trade-offs.

I now believe this is a High severity finding.

That said, please assign the appropriate severity based on these considerations. Thank you.

[clesaege]

The whole answer is non sense and it's hard to know if the hunter is very new to solidity or just delusional (note that I'm saying that literally, not sarcastically, I feel the answer may have been written by someone suffering from delusions of grandeur and I'd advise to get this checked).

Market is a public variable so solidity automatically creates a getter. Even if it weren't, you could find those through events/graph.

[kodakr]

Thanks for your honest reply and feedbk. Just discovered u are right about the public variable.