Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

No explicit check on marketPlaceName uniqueness in `SystemConfig: createMarketPlace` function

soupy

Summary

Lack of explicit condition check on marketPlaceName uniqueness in SystemConfig: createMarketPlace function causing the function to fail and revert with unclear error message creating confusion to user and potentially also affect any downstream processes or frontend UI that depends on the state of the market place creation.

Vulnerability Details

In the natspec mentioned in SystemConfig: createMarketPlace, the _marketPlaceName must be unique. However, there's no code implementation found within the function to address the market place name uniqueness.

/**
* @notice Create market place
* @param _marketPlaceName Market place name
* @param _fixedratio Fixed ratio
* @notice Caller must be owner
<@@> * @notice _marketPlaceName must be unique
* @notice _fixedratio is true if the market place is arbitration required
*/
function createMarketPlace(
string calldata _marketPlaceName,
bool _fixedratio
) external onlyOwner {
address marketPlace = GenerateAddress.generateMarketPlaceAddress(
_marketPlaceName
);
MarketPlaceInfo storage marketPlaceInfo = marketPlaceInfoMap[
marketPlace
];
​
if (marketPlaceInfo.status != MarketPlaceStatus.UnInitialized) {
revert MarketPlaceAlreadyInitialized();
}
​
marketPlaceInfo.status = MarketPlaceStatus.Online;
marketPlaceInfo.fixedratio = _fixedratio;
​
emit CreateMarketPlaceInfo(_marketPlaceName, marketPlace, _fixedratio);
}
​

Even though this function will still revert if same marketplace name is used again to call on SystemConfig: createMarketPlace, the revert of the function will return unclear message OwnableUnauthorizedAccount confusing the user and affects any downstream processes/front end that depends on the state of the function call / its revert message.

Proof of concept:
In test/PreMarkets.t.sol, add the following test :

function test_audit_systemConfig_createMarketPlace_uniqueMarketPlaceName() public {
// in the setUp() function of test file `test/PreMarkets.t.sol`, a marketplace with name "Backpack" has already been created
// a recreation of market place with the same name is executed below to demonstrate the test will revert with error message which is not clear that the process failed due to duplication of marketplace name
systemConfig.createMarketPlace("Backpack", false);
​
}
​

After a forge test run, the test will fail with the console displays log as below:

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[FAIL. Reason: OwnableUnauthorizedAccount(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)] test_audit_systemConfig_createMarketPlace_uniqueMarketPlaceName() (gas: 13210)
Traces:
[15348010] PreMarketsTest::setUp()
├─ [482674] → new WETH9@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← [Return] 2075 bytes of code
├─ [990557] → new TadleFactory@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ └─ ← [Return] 4836 bytes of code
​
.....
​
[13210] PreMarketsTest::test_audit_systemConfig_createMarketPlace_uniqueMarketPlaceName()
├─ [7898] UpgradeableProxy::createMarketPlace("Backpack", false)
│ ├─ [2864] SystemConfig::createMarketPlace("Backpack", false) [delegatecall]
│ │ └─ ← [Revert] OwnableUnauthorizedAccount(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)
│ └─ ← [Revert] OwnableUnauthorizedAccount(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)
└─ ← [Revert] OwnableUnauthorizedAccount(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.68ms (93.42µs CPU time)
​
Ran 1 test suite in 1.62s (3.68ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/PreMarkets.t.sol:PreMarketsTest
[FAIL. Reason: OwnableUnauthorizedAccount(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)] test_audit_systemConfig_createMarketPlace_uniqueMarketPlaceName() (gas: 13210)
​
Encountered a total of 1 failing tests, 0 tests succeeded
FAIL
​

The function above is reverted with message related to OwnableUnauthorizedAccount which is neither clear not relatable to the actual cause due to duplication of marketplace name.

Impact

Function reverts with error that is confusing to user and potentially affects any downstream processes or front-end UI that depends on the function call state/revert message

Tools Used

Manual review with forge test

Recommendations

To implmenet secondary mapping to keep track of whether a marketplace name has been set previously.

In SystemConfigStorage.sol, add a new mapping as :

/// @dev tracking of market place name/its associated generated address, ensuring the uniqueness of each marketplace created
mapping(address => bool) public marketPlaceNameTrackingMap;
​
// adjust the length of __gap to 93, due to the add on of new mapping `marketPlaceNameTrackingMap`
uint256[93] private __gap;

Then do the following :

function createMarketPlace(
string calldata _marketPlaceName,
bool _fixedratio
) external onlyOwner {
address marketPlace = GenerateAddress.generateMarketPlaceAddress(
_marketPlaceName
);
​
+ if (marketPlaceNameTrackingMap[marketPlace]) {
+ revert("Marketplace name already exists!");
+ }
​
MarketPlaceInfo storage marketPlaceInfo = marketPlaceInfoMap[
marketPlace
];
​
if (marketPlaceInfo.status != MarketPlaceStatus.UnInitialized) {
revert MarketPlaceAlreadyInitialized();
}
​
marketPlaceInfo.status = MarketPlaceStatus.Online;
marketPlaceInfo.fixedratio = _fixedratio;
​
emit CreateMarketPlaceInfo(_marketPlaceName, marketPlace, _fixedratio);
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-Admin-Errors-Malicious

The following issues and its duplicates are invalid as admin errors/input validation/malicious intents are1 generally considered invalid based on [codehawks guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). If they deploy/set inputs of the contracts appropriately, there will be no issue. Additionally admins are trusted as noted in READ.ME they can break certain assumption of the code based on their actions, and

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.