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

Contrast Between Comments and Actual Implementation in `createOffer` Function

0xbeastboy

Github

Summary

The createOffer function is a critical component of the PreMarktes contract, responsible for creating offers within the marketplace. This function accepts several parameters that define the offer's characteristics, such as the amount, points, trade tax, and collateral rate. However, there are some diferences between comments and actual implementation of the function, leading to potential logical errors that could result in improper validation of user inputs.

Vulnerability Details

The comment clearly states that eachTradeTax should be less than EACH_TRADE_TAX_DECIMAL_SCALER. However, the comparison in the code only checks if eachTradeTax is greater than EACH_TRADE_TAX_DECIMAL_SCALER, allowing for the scenario where eachTradeTax is exactly equal to EACH_TRADE_TAX_DECIMAL_SCALER, which violates the stated requirement.

Similary, comment indicates that collateralRate should be greater than COLLATERAL_RATE_DECIMAL_SCALER. The current code only checks if collateralRate is less than COLLATERAL_RATE_DECIMAL_SCALER, thereby allowing the value to be exactly equal to COLLATERAL_RATE_DECIMAL_SCALER, which contradicts the intended validation logic.

See the following code:

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
);
}
​

Impact

First logics difference can lead to situations where the eachTradeTax is set to an unacceptable value, potentially disrupting the intended financial logic of the platform, such as overcharging fees or miscalculating trade tax.

Similary allowing a collateralRate that is equal to COLLATERAL_RATE_DECIMAL_SCALER could potentially result in insufficient collateralization of offers, exposing the system to increased risk if the collateral provided is not adequate to cover potential losses.

Tools Used

Manual Review

Recommendations

The comparison should be modified to use the >= operator instead of >, ensuring that any value equal to or greater than EACH_TRADE_TAX_DECIMAL_SCALER is rejected.

Also the second comparison should be changed to use the <= operator instead of <, ensuring that any value less than or equal to COLLATERAL_RATE_DECIMAL_SCALER is rejected

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-off-by-one-Trade-TAX-100%

Similar to issue #1323, Despite this off-by-one error of the intended check, the difference between 99% and 100% is minimal, so I believe whether or not 100% is allowed has minimal impact. Ultimately, takers should not be realistically creating offer with such tradeTax

Support

FAQs

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