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

createOffer uses the wrong offerID

pyro

Summary

createOffer uses the wrong offerId as it first updated the variable before inputting it into the array.

Vulnerability Details

createOffer first increases the global parameter - offerId and then assigns the new offerInfoMap[offerAddr] and stockInfoMap[stockAddr] to our already new offerId.

offerId = offerId + 1;
​
offerInfoMap[offerAddr] = OfferInfo({
//@audit H - we are inputing the wrong `offerId`, as it's updated above `offerId + 1`
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
});
​
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
//@sus why is stock type flipped ?
stockType: params.offerType == OfferType.Ask ? StockType.Bid : StockType.Ask,
authority: _msgSender(),
maker: makerAddr,
preOffer: address(0x0),
offer: offerAddr,
points: params.points,
amount: params.amount
});

This will in tern will mess up the who flow as stockInfo.id is used inside listOffer to verify that our offer has authority. However since we are using the new offerId when we generate the authority address it would be 0, or something different than the one generated for our stock.

address offerAddr = GenerateAddress.generateOfferAddress(stockInfo.id);
if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}

Impact

createOffer creates an offer with the wrong ID, which will mess up the whole flow.

Tools Used

Manual review

Recommendations

Update the offerId after it's used

- offerId = offerId + 1;
​
offerInfoMap[offerAddr] = OfferInfo({
//@audit H - we are inputing the wrong `offerId`, as it's updated above `offerId + 1`
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
});
​
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
//@sus why is stock type flipped ?
stockType: params.offerType == OfferType.Ask ? StockType.Bid : StockType.Ask,
authority: _msgSender(),
maker: makerAddr,
preOffer: address(0x0),
offer: offerAddr,
points: params.points,
amount: params.amount
});
​
+ 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.