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

Improper state update, will allow offer creators in Turbo mode to abort ask offers before settlement, which is not intended

dimah7

Summary

Offer creator's abort offer status is not updated correctly, when a subsequent trader buys a stock from him and later on lists it. This will allow the origin offer creator to be cancel the ask offer, if in Turbo mode, which is not intended design.

Vulnerability Details

Having a closer look at the PreMarkets::listOffer() function:

function listOffer(
address _stock,
uint256 _amount,
uint256 _collateralRate
) external payable {
...
​
StockInfo storage stockInfo = stockInfoMap[_stock];
if (_msgSender() != stockInfo.authority) {
revert Errors.Unauthorized();
}
​
OfferInfo storage offerInfo = offerInfoMap[stockInfo.preOffer];
MakerInfo storage makerInfo = makerInfoMap[offerInfo.maker];
​
/// @dev market place must be online
ISystemConfig systemConfig = tadleFactory.getSystemConfig();
MarketPlaceInfo memory marketPlaceInfo = systemConfig.getMarketPlaceInfo(makerInfo.marketPlace);
​
marketPlaceInfo.checkMarketPlaceStatus(block.timestamp, MarketPlaceStatus.Online);
​
if (stockInfo.offer != address(0x0)) {
revert OfferAlreadyExist();
}
​
if (stockInfo.stockType != StockType.Bid) {
revert InvalidStockType(StockType.Bid, stockInfo.stockType);
}
​
/// @dev change abort offer status when offer settle type is turbo
if (makerInfo.offerSettleType == OfferSettleType.Turbo) {
address originOffer = makerInfo.originOffer;
OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}

Reading the natspec and the code, it's understandable that if the initial offer's settle type is Turbo, it should change the abort status to SubOfferListed, following the abort offer status enum:

/**
* @dev Abort offer status
* @notice Initialized, SubOfferListed, Aborted
* @param Initialized offer not yet exist.
* @param SubOfferListed some one trade, and relist the offer
* @param Aborted order has been aborted
*/
enum AbortOfferStatus {
Initialized,
SubOfferListed,
Aborted
}

It means that someone already traded and relisted the offer, so the original creator should not be able to abort it. The abortAskOffer function confirms it:

function abortAskOffer(address _stock, address _offer) external {
...
if (offerInfo.abortOfferStatus != AbortOfferStatus.Initialized) {
revert InvalidAbortOfferStatus(
AbortOfferStatus.Initialized,
offerInfo.abortOfferStatus
);
}
...

However it can be noticed that the memory keyword is used when retrieving the origin offer's data:

OfferInfo memory originOfferInfo = offerInfoMap[originOffer];

Based on how Solidity works, this will not change any state.

Here is a scenario:

  1. Alice, creates an offer in Turbo mode = AbortOfferStatus.Initialized, means non-existent offer yet

  2. Bob takes it, and then lists the offer

  3. This should change the status to origin offer's status to = AbortOfferStatus.SubOfferListed, so it can't be a aborted following the abortAskOffer function

  4. Alice still can abort her offer, which unintended design

Impact

  • Low: as per CodeHawks docs for severity: "However, a function might be incorrect, the state might not be handled appropriately, etc". Also this is not the protocol's intended idea.

Tools Used

Manual Review

Recommendations

Change the memory keyword with the storage keyword, in the PreMarkets::listOffer():

/// @dev change abort offer status when offer settle type is turbo
if (makerInfo.offerSettleType == OfferSettleType.Turbo) {
address originOffer = makerInfo.originOffer;
- OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
+ OfferInfo storage originOfferInfo = offerInfoMap[originOffer];
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}
Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-listOffer-originIOfferInfo-storage-memory

Valid high severity, because the `abortOfferStatus` of the offer is not updated and persist through `storage` when listing an offer for turbo mode within the `offerInfoMap` mapping, it allows premature abortion given the `abortOfferStatus` defaults to `Initialized`, allowing the bypass of this [check](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L552-L557) here and allow complete refund of initial collateral + stealing of trade tax which can potentially be gamed for profits using multiple addresses

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.