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

Incorrect `offerId` Handling Leading to Address Collisions and Data Integrity Risks

n3smaro

Summary

The PerMarkets contract, inheriting from PerMarketsStorage, demonstrates a critical issue with the timing of offerId incrementation. The incrementation of offerId is performed after generating addresses and creating entities (offers, makers, stocks) based on the current offerId. This incorrect order of operations can lead to address collisions, mismatched data references, and overall data integrity risks.

Vulnerability Details

The offerId state variable is used for generating unique addresses and managing entity references in the PerMarkets contract. The typical workflow involves:

  1. Address Generation: Addresses for maker, offer, and stock are generated using the current offerId.

  2. Entity Creation: Entities (makers, offers, and stocks) are created using the addresses generated in the previous step and the new offerId value will be the id attribute for each new entity.

  3. Incrementation: The offerId is incremented to prepare for the next entity.

The issue in the createOffer and createTaker functions involves a mismatch between the offerId used for generating addresses and the offerId actually stored in the contract.

In the createOffer function, addresses for the maker, offer, and stock are generated using the current offerId before it is incremented. This means that the new addresses are based on the old offerId, but the updated offerId is stored only after the addresses are generated. Consequently, if multiple offers are created quickly in succession, this mismatch can lead to incorrect associations between addresses and offer data.

/// @dev generate address for maker, offer, stock.
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;
​
{
/// @dev transfer collateral from _msgSender() to capital pool
uint256 transferAmount = OfferLibraries.getDepositAmount(
params.offerType,
params.collateralRate,
params.amount,
true,
Math.Rounding.Ceil
);
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.tillIn{value: msg.value}(
_msgSender(),
params.tokenAddress,
transferAmount,
false
);
}
​
/// @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
});

Similarly, in the createTaker function, a new stockAddr is generated using the current offerId, but the offerId is incremented only after updating the stock information. This can result in the stock being linked to the wrong offerId, causing inconsistencies in data storage and retrieval.

/// @dev update stock info
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;

Impact

  • Address Collisions: Incorrect addresses may lead to conflicts and unintended overwriting of existing entities.

  • Data Integrity Issues: Mismatched references can corrupt the mapping of offers, stocks, and other entities, affecting the reliability of contract operations.

  • Operational Disruptions: Users may encounter issues where their operations reference incorrect or stale entities, leading to failed transactions and disputes.

Tools Used

Manual Code Review

Recommendations

Ensure that the offerId is incremented before generating addresses and creating new entities. This will ensure that all addresses are unique and correctly assigned.

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.