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

ID Collision in stockInfo and orderInfo entries

oxelmiguel

Summary

The stockInfo.id and offerInfo.id, which are supposed to be unique, can be duplicated, leading to potential issues in the system.

Vulnerability Details

  • When creating an offer with PreMarkets::createOffer, the global offerId is used to generate addresses before the offerId is incremented:

address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
​
if (makerInfoMap[makerAddr].authority != address(0x0)) {
revert MakerAlreadyExist();
}
​
if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}
​
if (stockInfoMap[stockAddr].authority != address(0x0)) {
revert StockAlreadyExist();
}
​
offerId = offerId + 1;
​

Later in the same function, the offerId is used as the ID for both OfferInfo and stockInfo:

/// @dev update offer info
offerInfoMap[offerAddr] = OfferInfo({
id: offerId,
authority: _msgSender(),
maker: makerAddr,
offerStatus: OfferStatus.Virgin,
offerType: params.offerType,
points: params.points,
amount: params.amount,
collateralRate: params.collateralRate,
abortOfferStatus: AbortOfferStatus.Initialized,
usedPoints: 0,
tradeTax: 0,
settledPoints: 0,
settledPointTokenAmount: 0,
settledCollateralAmount: 0
});
​
/// @dev update stock info
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
stockType: params.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: makerAddr,
preOffer: address(0x0),
offer: offerAddr,
points: params.points,
amount: params.amount
});
​

However, the offerId is not incremented further, which can lead to the same ID being used for multiple entries.

the full code here : https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L39

  • In the PreMarkets::createTaker function, a new stock is created with the offerId, which is then incremented:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L239

stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
stockType: offerInfo.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: offerInfo.maker,
preOffer: _offer,
points: _points,
amount: depositAmount,
offer: address(0x0)
});
​
offerId = offerId + 1;

At this point, there is a risk that two StockInfo entries could end up with the same ID

  • In the listOffer function, stockInfo.id is used to generate a new offerAddr and ID:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L370

address offerAddr = GenerateAddress.generateOfferAddress(stockInfo.id);
if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}
/// @dev update offer info
offerInfoMap[offerAddr] = OfferInfo({
id: stockInfo.id,
authority: _msgSender(),
maker: offerInfo.maker,
offerStatus: OfferStatus.Virgin,
offerType: offerInfo.offerType,
abortOfferStatus: AbortOfferStatus.Initialized,
points: stockInfo.points,
amount: _amount,
collateralRate: _collateralRate,
usedPoints: 0,
tradeTax: 0,
settledPoints: 0,
settledPointTokenAmount: 0,
settledCollateralAmount: 0
});

This can result in two OfferInfo entries having the same ID, leading to potential conflicts.

Proof of Concept

  1. user A create an offer with offerId = 1

    • offerAddr, stockAddr and makerAddr are generated with =1

    • offerId is then incremented (offerId = 2)

    • stockInfo.id = 2

    • offerInfo.id = 2

  2. user B create a taker with offerId = 2

    • stockInfo.id = 2

    • offerId is then update (offerId = 3)

  3. user B list offer (using the stock he just create)

    • offerInfo.id = 2

Impact

Duplicate IDs in stockInfo and offerInfo can compromise the integrity of the data.

Recommendations

Modify the createOffer logic to increment the offerId only after both offerInfo and stockInfo entries have been successfully created. This will ensure that each offerId is unique and used consistently throughout the process, preventing collisions.

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.