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

Inconsistent `offerId` usage in Address Generation and Data Storage in `PreMarktes::createOffer`

0x5chn0uf

Summary

In the PreMarkets::createOffer function, there is a mismatch between the offerId used for address generation and the id stored in the offer and stock structs.

Vulnerability Details

The function generates addresses using the current offerId, then increments offerId, and finally stores data using the incremented value.

This leads to a situation where for any given offer, its address is generated using one ID, while its stored data uses the next ID.

address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
​
// ...
​
offerId = offerId + 1;
​
// ...
​
makerInfoMap[makerAddr] = MakerInfo({...});
offerInfoMap[offerAddr] = OfferInfo({
id: offerId,
...
});
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
...
});

Whereas in PreMarktes::createTaker, the correct logic is used. This is inconsistent.

function createTaker(address _offer, uint256 _points) external payable {
// ...
address stockAddr = GenerateAddress.generateStockAddress(offerId);
// ...
​
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
...
});
​
offerId = offerId + 1;
// ...
}

Impact

While this inconsistency doesn't immediately break the contract's functionality, it introduces several concerns:

  • Data Inconsistency and Reduced Auditability: The id stored in structs doesn't match the offerId used to generate their addresses, making the contract harder to audit, debug, and maintain.

  • Potential for Future Issues: This mismatch could lead to unexpected behavior or subtle bugs that are hard to detect, especially if new functions are added or the protocol is upgraded or integrated with other systems.

  • Violation of Best Practices: The inconsistency goes against the principle of least astonishment in code design, potentially leading to confusion during future development or maintenance efforts.

Tools Used

Manual Review

Recommendations

To resolve this inconsistency and improve the contract's clarity and maintainability, increment offerId after all operations in createOffer:

address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
​
- offerId = offerId + 1;
​
makerInfoMap[makerAddr] = MakerInfo({...});
offerInfoMap[offerAddr] = OfferInfo({
id: offerId,
...
});
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
...
});
​
+ offerId = offerId + 1;
​
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-createOffer-offerId-increment-after

I believe this is valid low severity, although there is inconsistency here when using the correct `offerId` for assigning offerIds and generating the unique addresses as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L67-L69), this is purely an accounting error for offerIds. If we generate the offerId using current `offerId - 1`, the appropriate listing/taker orders can still be created against those offers.

Support

FAQs

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