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

Incorrect offerId set in info mappings

yotov721

Summary

Incorrect offerId set in info mappings leads DoS or overwriting other users offers

Vulnerability Details

The protocol offers the ability for users to create bid-ask trades. When a user creates a new offer 3 mappings are used to store data about the offer and the stock. The mappings have an address as a key. This address is generated for a string and a uint256 offerId which is stored in the PreMarkets contract and incremented every time a new offer is made.

If we take a look at the PreMarkets::createOffer function it uses offerId to generate the maker offer and stock addresses. Then it is incremented and finally the objects are saved in the mappings with the newly incremented offerId which is a problem.

function createOffer(CreateOfferParams calldata params) external payable {
/**
* @dev points and amount must be greater than 0
* @dev eachTradeTax must be less than 100%, decimal scaler is 10000
* @dev collateralRate must be more than 100%, decimal scaler is 10000
*/
if (params.points == 0x0 || params.amount == 0x0) {
revert Errors.AmountIsZero();
}
​
if (params.eachTradeTax > Constants.EACH_TRADE_TAX_DECIMAL_SCALER) {
revert InvalidEachTradeTaxRate();
}
​
if (params.collateralRate < Constants.COLLATERAL_RATE_DECIMAL_SCALER) {
revert InvalidCollateralRate();
}
​
/// @dev market place must be online
ISystemConfig systemConfig = tadleFactory.getSystemConfig();
MarketPlaceInfo memory marketPlaceInfo = systemConfig
.getMarketPlaceInfo(params.marketPlace);
marketPlaceInfo.checkMarketPlaceStatus(
block.timestamp,
MarketPlaceStatus.Online
);
​
/// @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
});
​
emit CreateOffer(
offerAddr,
makerAddr,
stockAddr,
params.marketPlace,
_msgSender(),
params.points,
params.amount
);
}

It is a problem because if we take a look at listOffer it uses the stockInfo.id to generate na new offer address and checks if it is in use by checking the offer info authority against address(0). Finally it creates a new offer with the same id. This condition would revert every single time, when there is an offer created after the current one.

=> address offerAddr = GenerateAddress.generateOfferAddress(stockInfo.id);
=> if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}
​
/// @dev update offer info
offerInfoMap[offerAddr] = OfferInfo({
=> id: stockInfo.id,
authority: _msgSender(),
maker: offerInfo.maker,
offerStatus: OfferStatus.Virgin,
offerType: offerInfo.offerType,
abortOfferStatus: AbortOfferStatus.Initialized,
points: stockInfo.points,
amount: _amount,
collateralRate: _collateralRate,
usedPoints: 0,
tradeTax: 0,
settledPoints: 0,
settledPointTokenAmount: 0,
settledCollateralAmount: 0
});

Add the following test to PreMarkets.t.sol and run with forge test -vvv --mt test_wrong_id_set

function test_wrong_id_set() public {
vm.startPrank(user);
console2.log(preMarktes.offerId()); // 0
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
console2.log(preMarktes.offerId()); // 1
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
console2.log(preMarktes.offerId()); // 2
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
OfferInfo memory offerInfo0 = preMarktes.getOfferInfo(offerAddr);
address offerAddr1 = GenerateAddress.generateOfferAddress(1);
OfferInfo memory offerInfo1 = preMarktes.getOfferInfo(offerAddr1);
console2.log(offerInfo0.authority); // both offers exist
console2.log(offerInfo1.authority); // both offers exist
​
address stock1Addr = GenerateAddress.generateStockAddress(0);
// stock with address generated from 0 will have an id of 1 because of the bug, but offer with id 1 already exists
​
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
vm.stopPrank();
}

Impact

DoS

Tools Used

Manual Review

Recommendations

Increment the offer id after the info mappings are created

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;
​
{
/// @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
});
​
+ offerId = offerId + 1;
​
emit CreateOffer(
offerAddr,
makerAddr,
stockAddr,
params.marketPlace,
_msgSender(),
params.points,
params.amount
);
}

Note that after fixing this bug listOffer would also have to be fixed because the offer authority would be the original offer authority. Hence you could just modify listOffer by

- address offerAddr = GenerateAddress.generateOfferAddress(stockInfo.id);
+ address offerAddr = GenerateAddress.generateOfferAddress(offerId);
+ offerId++;
if (offerInfoMap[offerAddr].authority != address(0x0)) {
revert OfferAlreadyExist();
}
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.