Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

The createOffer function is messing up data by registering wrong IDs.

mikebello

Summary

The createOffer function in the PreMarkets contracts is registering wrong ID data in the offerInfoMap and the stockInfoMap this would mess up the data in the struct by always registering a different ID of which was used to calculate the address of these structs, and will duplicate the ID registered in different offerInfoMap and the stockInfoMap structs.

Vulnerability Details

When a user creates an offer to sell/buy points the createOffer function is called, this function uses the variable offerId to count the number of orders that have been created in the market and differentiate the orders by creating unique addresses for the offerInfoMap, stockInfoMap and the makerInfoMap structs based on the offerId variable.

/// @dev generate address for maker, offer, stock.
address makerAddr = GenerateAddress.generateMakerAddress(offerId);
address offerAddr = GenerateAddress.generateOfferAddress(offerId);
address stockAddr = GenerateAddress.generateStockAddress(offerId);

then, it validates these addresses don't exist already

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;

and later, these addresses are used to register all the data these structs need for the system to work as expected, as you can see below, one other data needed is the ID for the offerInfoMap and the stockInfoMap structs, which should register the offerId value used to calculate the addresses, but this offerId variable is being increased in the previous step, so the offerId value register at this point is wrong because it will point to an existing offerId at the moment.

/// @dev update maker info
makerInfoMap[makerAddr] = MakerInfo({
offerSettleType: params.offerSettleType,
authority: _msgSender(),
marketPlace: params.marketPlace,
tokenAddress: params.tokenAddress,
originOffer: offerAddr,
platformFee: 0,
eachTradeTax: params.eachTradeTax
});
​
/// @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
});

I created a basic test and added some console logs to the CreateOffer, CreatTaker and listOffer functions to show that the ID registered in this function is wrong, duplicated, and some IDs are skipped.

function test_Bad_ID_Assignation_CreateOffer() public {
vm.startPrank(user);
// creating first offer
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 0.01 * 1e6, 12000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
// creating firs taker
preMarktes.createTaker(offerAddr, 500);
​
// listing the points bougth
address stock1Addr = GenerateAddress.generateStockAddress(1); // address 1
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
vm.stopPrank();
​
vm.startPrank(user);
// creating a 2° offer
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 0.01 * 1e6, 12000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
vm.stopPrank();
}

This is the result of the test:

% forge test --mt test_Bad_ID_Assignation_CreateOffer -vvv
[⠊] Compiling...
[⠰] Compiling 1 files with Solc 0.8.25
[⠔] Solc 0.8.25 finished in 2.41s
Compiler run successful!
​
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_Bad_ID_Assignation_CreateOffer() (gas: 1476021)
Logs:
// first create offer call
offer stockAddr - Create Offer: 0xE619a2899a8db14983538159ccE0d238074a235d
offer id - Offer: 1 // wrong ID register in offer struct
stock stockAddr - Create Offer: 0x41e7A7cD0C389cD6015D23df7A112c6CC19A192F
stock id - Offer: 1 // wrong ID register in stock struct
​
// create taker call
stock stockAddr - CreateTaker: 0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8
stock id - CreateTaker: 1 // same ID registered in stock struct for the create taker
​
// list offer call
stock ID - list offer: 1
Address for offer - list offer: 0xec19aCA7DE739Aeff3d1924151F084bF67920a9a
​
// Second create offer call
offer stockAddr - Create Offer: 0x1d17E6d73681612CDDFDb615c68117aD23f8d438
offer id - Offer: 3 // wrong ID register in offer struct
stock stockAddr - Create Offer: 0xDccDBCD5F1429903af9aC7a6b7adb68242EcbD46
stock id - Offer: 3 // wrong ID register in stock struct
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.99ms (2.61ms CPU time)

Impact

The offerInfoMap and the stockInfoMap structs are saving the wrong ID, this will duplicate IDs in different orders and skip some IDs.

Tools Used

Manual Review

Recommendations

Update the offerId variable after it is assigned in the creation of the offerInfoMap and the stockInfoMapstructs, like. you do in the createTaker function.

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months 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.