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

Inconsistent ID Setting in createTaker() and createOffer() Functions

Tomas0707

Summary

The createTaker() function in the PreMarkets.sol contract sets the id of the StockInfo structure before incrementing the offerId. In contrast, the createOffer() function increments the offerId before setting the id. This inconsistency can potentially lead to the same ID being assigned to multiple entities or an ID not being set correctly.

Vulnerability Details

createTaker() function. The id for the StockInfo structure is set before the offerId is incremented.

stockInfoMap[stockAddr] = StockInfo({
id: offerId,
...
});
offerId = offerId + 1;
​

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

createOffer() function. The offerId is incremented before setting the id.

offerId = offerId + 1;
...
offerInfoMap[offerAddr] = OfferInfo({
id: offerId,
...
});

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

This discrepancy can lead to one ID being assigned to two different entities or, in some cases, not being assigned at all.

Impact

The inconsistency in ID assignment can cause unexpected behavior in the system, such as ID Collision or Incorrect Data Mapping.

Proof of Concept

This is the test code.

function test_id() public {
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.startPrank(user1);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.prank(user2);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
2000,
0.02 * 1e18,
15000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
console2.log("offer(0): ", GenerateAddress.generateOfferAddress(0));
console2.log("id: ", preMarktes.getOfferInfo(
GenerateAddress.generateOfferAddress(0)
).id);
console2.log("stock(0): ", GenerateAddress.generateStockAddress(0));
console2.log("id: ", preMarktes.getStockInfo(
GenerateAddress.generateStockAddress(0)
).id);
console2.log("stock(1): ", GenerateAddress.generateStockAddress(1));
console2.log("id: ", preMarktes.getStockInfo(
GenerateAddress.generateStockAddress(1)
).id);
console2.log("offer(2): ", GenerateAddress.generateOfferAddress(2));
console2.log("id: ", preMarktes.getOfferInfo(
GenerateAddress.generateOfferAddress(2)
).id);
console2.log("stock(2): ", GenerateAddress.generateStockAddress(2));
console2.log("id: ", preMarktes.getStockInfo(
GenerateAddress.generateStockAddress(2)
).id);
}

The result is like this.

offer(0): 0xE619a2899a8db14983538159ccE0d238074a235d
id: 1
stock(0): 0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F
id: 1
stock(1): 0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8
id: 1
offer(2): 0x1d17E6d73681612CDDFDb615c68117aD23f8d438
id: 3
stock(2): 0xDccDBCD5F1429903af9aC7a6b7adb68242EcbD46
id: 3

As you can see, ID 1 is assigned to two different stocks and ID 2 is not assigned at all.

Tools Used

Manual code review

Recommendations

Increment offerId before setting the id in the createTaker() function, similar to how it is done in the createOffer() function.

offerId = offerId + 1;
​
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)
});
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.