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

Improper `offerId` Management Can Lead to Address Collisions and Data Overwrites (`PreMarkets::createOffer`, `PreMarkets::listOffer`, `PreMarkets::createTaker`)

4gontuk

Summary

Vulnerability Detail

The PreMarkets contract is designed to facilitate the creation, listing, and management of market offers. The createOffer(), listOffer(), and createTaker() functions are central to this functionality, each generating unique addresses for makers, offers, and stocks using a shared offerId. However, the current implementation does not ensure the uniqueness of offerId, leading to potential address collisions and unintended overwrites in the mappings (makerInfoMap, offerInfoMap, stockInfoMap).

In createOffer(), addresses for the maker, offer, and stock are generated using the current offerId. The offerId is then incremented by 1. If the offerId is reused or not properly managed, it can lead to the generation of the same addresses, causing unintended overwrites in the mappings. Similar logic is used in listOffer() and createTaker(), where new addresses are generated using the offerId or stockInfo.id.

The root cause of the issue is the lack of a mechanism to ensure that offerId is unique and that the generated addresses do not already exist in the mappings. This can lead to significant issues, including loss of data integrity, unauthorized access, and potential financial losses.

Relevant code snippets:

function createOffer(CreateOfferParams calldata params) external payable {
// Generate addresses for maker, offer, stock
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
​
// Check for existing entries
if (makerInfoMap[makerAddr].authority != address(0x0)) {
revert MakerAlreadyExist();
}
​
if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}
​
if (stockInfoMap[stockAddr].authority != address(0x0)) {
revert StockAlreadyExist();
}
​
// Increment offerId
offerId = offerId + 1;
​
// ... existing code ...
}

Impact

Address collisions and unintended overwrites in the mappings can lead to significant issues, including loss of data integrity, unauthorized access, and potential financial losses. Ensuring unique offerId and proper address management is critical to maintaining the security and functionality of the smart contract.

Proof of Concept

  1. User A calls createOffer() and generates addresses for maker, offer, and stock using offerId = 1.

  2. The offerId is incremented to 2 after the addresses are generated.

  3. Due to a bug or manipulation, the offerId is reset to 1.

  4. User B calls createOffer() and generates addresses for maker, offer, and stock using offerId = 1.

  5. The addresses generated for User B collide with those of User A, leading to unintended overwrites in the mappings (makerInfoMap, offerInfoMap, stockInfoMap).

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that offerId is unique and properly managed to avoid reuse and collisions. Before generating new addresses, check if the generated addresses already exist in the mappings to prevent overwrites. Here is a suggested fix for the createOffer() function (Not sure):

function createOffer(CreateOfferParams calldata params) external payable {
// Ensure unique offerId
uint256 newOfferId = offerId;
address makerAddr;
address offerAddr;
address stockAddr;
do {
newOfferId++;
makerAddr = GenerateAddress.generateMakerAddress(newOfferId);
offerAddr = GenerateAddress.generateOfferAddress(newOfferId);
stockAddr = GenerateAddress.generateStockAddress(newOfferId);
} while (
makerInfoMap[makerAddr].authority != address(0) ||
offerInfoMap[offerAddr].authority != address(0) ||
stockInfoMap[stockAddr].authority != address(0)
);
offerId = newOfferId;
// ... rest of the function ...
}

Apply similar changes to listOffer() and createTaker() functions. Additionally, consider implementing a more robust ID generation system, such as using keccak256 hashes of concatenated parameters to ensure uniqueness.

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.