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

Inconsistent logic in updating PreMarkets::offerId causes two stock addresses to map to the same Offer ID

ragnarok

Vulnerability Details

In the PreMarkets::createOffer function, the PreMarkets::offerId variable is used when generating the address for Stock. However, when saving StockInfo, the value of the id field is assigned to PreMarkets::offerId + 1. In the PreMarkets::createTaker function, the PreMarkets::offerId variable is used for both generating the address for Stock and saving StockInfo,

PreMarkets::createOffer

function createOffer(CreateOfferParams calldata params) external payable {
...
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
...
offerId = offerId + 1;
...
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
...
});
}

PreMarkets::createTaker

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

Impact

This is not considered best practice and can introduce potential vulnerabilities when using StockInfo::id to regenerate an address.

Proof of Code

Add the following test case into the file PreMarkets.t.sol

function test_twoStockAddressMappingSameId() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
address stock0Addr = GenerateAddress.generateStockAddress(0);
address stock1Addr = GenerateAddress.generateStockAddress(1);
StockInfo memory stock0 = preMarktes.getStockInfo(stock0Addr);
StockInfo memory stock1 = preMarktes.getStockInfo(stock1Addr);
​
assertEq(stock0.id, stock1.id);
}

Recommendations

Update PreMarkets::offerId before generating new address for Stock or after saving the StockInfo. This ensures that the value of the id field in StockInfo corresponds with the value of the PreMarkets::offerId variable used to generate the address for Stock.

function createOffer(CreateOfferParams calldata params) external payable {
...
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);
...
- offerId = offerId + 1;
...
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
...
});
+ 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.