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

Low risk report for Tadle

k42

Comprehensive Low Risk Report for Tadle by K42

Summary Table of all Lows

ID Title Severity Impact Likelihood Contract
L-01 Risk of precision loss:OfferLibraries.getDepositAmount() Low Medium Low OfferLibraries.sol
L-02 No upper bound check:eachTradeTax.PreMarkets.createOffer() Low Medium Low PreMarkets.sol
L-03 Need better checks for token addresses :TokenManager Low Medium Low TokenManager.sol
L-04 Wrong market status: MarketPlaceLibraries Low Medium Low MarketPlaceLibraries.sol
L-05 No slippage protection: DeliveryPlace.closeBidOffer() Low Medium Low DeliveryPlace.sol
L-06 Not optimzed for gas: CapitalPoo.approve() Low Low Medium CapitalPool.sol
L-07 Not effective rounding: OfferLibraries Low Low Medium OfferLibraries.sol
L-08 No upper bound check for platform fee rate: SystemConfig Low Medium Low SystemConfig.sol
L-09 Need better validation: SystemConfig Low Low Medium SystemConfig.sol

[L-01] Risk of precision loss:OfferLibraries.getDepositAmount()

Contract

OfferLibraries.sol

Summary

OfferLibraries has precision loss issue in the getDepositAmount function, which could lead to small, but still inaccuracies in deposit calculations.

Vulnerability Details

In the getDepositAmount function, the calculation of the deposit amount for certain offer types involves multiplication followed by division:

return
Math.mulDiv(
_amount,
_collateralRate,
Constants.COLLATERAL_RATE_DECIMAL_SCALER,
_rounding
);

Math.mulDiv function is already used to mitigate some precision loss, but not for small rounding errors due to the order of operations and the use of integer division.

Impact

Small differences in deposit amounts being calculated. In most cases, the difference would be very minimal, but over many transactions or with very large amounts, it could lead to discrepancies between expected and actual deposit amounts.

Proof of Concept

​
function testPrecisionLoss() public {
uint256 amount = 1e18;
uint256 collateralRate = 15000; // 150%
uint256 expectedResult = 1.5e18;
uint256 result = OfferLibraries.getDepositAmount(
OfferType.Ask,
collateralRate,
amount,
true,
Math.Rounding.Down
);
assertEq(result, expectedResult, "Precision loss occurred");
// Demonstrate precision loss with larger numbers
amount = 1e27;
expectedResult = 1.5e27;
result = OfferLibraries.getDepositAmount(
OfferType.Ask,
collateralRate,
amount,
true,
Math.Rounding.Down
);
assertTrue(result != expectedResult, "No precision loss with larger numbers");
}
​

Recommendations

Best to use higher precision for the calculations. Make it as accurate as possible. One approach to do this, is to use a larger scale factor for _collateralRate and Constants.COLLATERAL_RATE_DECIMAL_SCALER

uint256 private constant PRECISION_FACTOR = 1e6;
​
return
Math.mulDiv(
_amount,
_collateralRate * PRECISION_FACTOR,
Constants.COLLATERAL_RATE_DECIMAL_SCALER * PRECISION_FACTOR,
_rounding
);

Above allows for precise intermediate, results before the finality of rounding is applied.

[L-02] No upper bound check:eachTradeTax.PreMarkets.createOffer()

Contract

PreMarkets.sol

Summary

PreMarket uses no upper bound check for the eachTradeTax parameter, in the createOffer function, allowing a path for unreasonably high trade taxes.

Vulnerability Details

createOffer function, uses a if statement to make sure that eachTradeTax is not greater than Constants.EACH_TRADE_TAX_DECIMAL_SCALER:

if (params.eachTradeTax > Constants.EACH_TRADE_TAX_DECIMAL_SCALER) {
revert InvalidEachTradeTaxRate();
}

The low bug is in the fact, this only prevents the eachTradeTax from exceeding 100% (say EACH_TRADE_TAX_DECIMAL_SCALER is 10000). Using no check to prevent setting an sily high trade tax, such as 99.99%.

Impact

Allowing extremely high trade taxes could lead to user annoyance, and imbalances in the system. Users might unknowingly create offers with exorbitant trade taxes, leading to some losses or disputes.

Proof of Concept

​
function testHighTradeTax() public {
CreateOfferParams memory params = CreateOfferParams({
marketPlace: address(0),
tokenAddress: address(0),
points: 100,
amount: 1e18,
collateralRate: 15000,
eachTradeTax: 9999, // 99.99%
offerType: OfferType.Ask,
offerSettleType: OfferSettleType.Protected
});
​
// This should not revert
preMarkets.createOffer(params);
​
// But it allows for an extremely high trade tax
assertTrue(params.eachTradeTax > 5000, "Trade tax should be unreasonably high");
}
}

Recommendations

Just add an additional check to set a reasonable upper limit for eachTradeTax:

uint256 private constant MAX_TRADE_TAX = 1000; // 10% as the maximum trade tax
​
function createOffer(CreateOfferParams calldata params) external payable {
// here is a check for this
if (params.eachTradeTax > MAX_TRADE_TAX) {
revert ExcessiveTradeWaxRate();
}
// and so on
}

Above validates that the trade tax cannot exceed a reasonable maximum (in this example, 10%), protecting users from abusive settings.

[L-03] Need better checks for token addresses :TokenManager

Contract

TokenManager.sol

Summary

TokenManager doest have good enough validation for token addresses, when updating the token whitelist, this enables a path, allowing non-contract addresses to be whitelisted. Therefore unintended behaviour.

Vulnerability Details

updateTokenWhiteListed function, doesn't use a check to validate that the addresses being whitelisted are the valid token contract addresses:

function updateTokenWhiteListed(
address[] calldata _tokens,
bool _isWhiteListed
) external onlyOwner {
uint256 _tokensLength = _tokens.length;
​
for (uint256 i = 0; i < _tokensLength; ) {
_updateTokenWhiteListed(_tokens[i], _isWhiteListed);
unchecked {
++i;
}
}
}

Impact

Non-contract addresses/ invalid token contracts being whitelisted anyway. .

Proof of Concept

function testInvalidTokenWhitelisting() public {
address[] memory tokens = new address[](2);
tokens[0] = address(0);
tokens[1] = address(this);
​
// This should not revert, but it's allowing non-ERC20 addresses
tokenManager.updateTokenWhiteListed(tokens, true);
​
assertTrue(tokenManager.tokenWhiteListed(address(0)), "Zero address should not be whitelisted");
assertTrue(tokenManager.tokenWhiteListed(address(this)), "Non-ERC20 contract should not be whitelisted");
}
}

Recommendations

Best to use better handling that asserts each address being whitelisted is a contract address, and uses the ERC20 interface.

import "@openzeppelin/contracts/utils/Address.sol";
import "@openzeppelin/contracts/interfaces/IERC20.sol";
​
function _updateTokenWhiteListed(address _token, bool _isWhiteListed) internal {
require(Address.isContract(_token), "Address is not a contract");
require(IERC20(_token).totalSupply() >= 0, "Not an ERC20 token");
tokenWhiteListed[_token] = _isWhiteListed;
emit UpdateTokenWhiteListed(_token, _isWhiteListed);
}

^Only valid ERC20 token contracts can be whitelisted.

[L-04] Wrong market status: MarketPlaceLibraries

Contract

MarketPlaceLibraries.sol

Summary

MarketPlaceLibraries.getMarketPlaceStatus allows returning a wrong status if the tge time is set to a future date.

Vulnerability Details

Low bug is because the function checks the current block timestamp against the tge and settlementPeriod to determine the market status:

if (
_blockTimestamp >
_marketPlaceInfo.tge + _marketPlaceInfo.settlementPeriod
) {
return MarketPlaceStatus.BidSettling;
}
​
if (_blockTimestamp > _marketPlaceInfo.tge) {
return MarketPlaceStatus.AskSettling;
}
​
return _marketPlaceInfo.status;

But the issues occurs, if the tge is set to a future date, the function will always return the current status, even if it's intended to be different before the TGE.

Impact

Unintended returns, if the system relies on accurate market statuses before the TGE occurs. This can allow or prevent certain actions based on a wrong market status.

Proof of Concept

function testInaccurateMarketStatus() public {
MarketPlaceInfo memory info = MarketPlaceInfo({
fixedratio: false,
status: MarketPlaceStatus.Online,
tokenAddress: address(0),
tokenPerPoint: 1e18,
tge: block.timestamp + 1 days,
settlementPeriod: 7 days
});
​
MarketPlaceStatus status = MarketPlaceLibraries.getMarketPlaceStatus(block.timestamp, info);
assertEq(uint(status), uint(MarketPlaceStatus.Online), "Status should be Online but is incorrect");
​
// It should be in a pre-TGE state, but the function doesn't account for this
}
​

Recommendations

To solive it, add code below to account for the case where the TGE is in the future:

function getMarketPlaceStatus(
uint256 _blockTimestamp,
MarketPlaceInfo memory _marketPlaceInfo
) internal pure returns (MarketPlaceStatus _status) {
if (_marketPlaceInfo.status == MarketPlaceStatus.Offline) {
return MarketPlaceStatus.Offline;
}
​
if (_marketPlaceInfo.tge == 0) {
return _marketPlaceInfo.status;
}
​
if (_blockTimestamp < _marketPlaceInfo.tge) {
return MarketPlaceStatus.PreTGE; // New status for before TGE
}
​
if (_blockTimestamp > _marketPlaceInfo.tge + _marketPlaceInfo.settlementPeriod) {
return MarketPlaceStatus.BidSettling;
}
​
return MarketPlaceStatus.AskSettling;
}

Above code, introduces a new PreTGE status and ensures the function returns the correct status at all times, this is best.

[L-05] No slippage protection: DeliveryPlace.closeBidOffer()

Contract

DeliveryPlace.sol

Summary

DeliveryPlace.closeBidOffer has no slippage protection.

Vulnerability Details

In closing a bid offer, the refund amount is calculated based on the current state:

uint256 refundAmount = OfferLibraries.getRefundAmount(
offerInfo.offerType,
offerInfo.amount,
offerInfo.points,
offerInfo.usedPoints,
offerInfo.collateralRate
);

The occurs becasue of the handling for the user to specify a minimum acceptable refund amount. The refund amount could be lower than expected by the time the transaction is processed.

Impact

Users closing bid offers receive less refund than should, if market conditions change rapidly between transaction submission and execution. Which they will, at some point. Therefore not mitigating this, opens up financial losses for users, especially the in higher-value transactions.

Proof of Concept

​
function testNoSlippageProtection() public {
// Setup mock data
address offer = address(1);
uint256 expectedRefund = 1e18;
​
// Mock the getOfferInfo function to return test data
// This would require more setup in a real test
​
// This should not revert, but it doesn't protect against slippage
deliveryPlace.closeBidOffer(offer);
​
// In a real scenario, the refund amount could be much lower than expected
// without the user being able to specify a minimum acceptable amount
}

Recommendations

Please add slippage protection by adding a minRefundAmount parameter to closeBidOffer :

function closeBidOffer(address _offer, uint256 _minRefundAmount) external {
// checks good here
​
uint256 refundAmount = OfferLibraries.getRefundAmount(
offerInfo.offerType,
offerInfo.amount,
offerInfo.points,
offerInfo.usedPoints,
offerInfo.collateralRate
);
​
require(refundAmount >= _minRefundAmount, "Refund amount too low");
// same
}

Opens users to choose the minimum refund they're willing to accept, protecting them from un-neccassry losses due to market.

[L-06] Not optimzed for gas: CapitalPool.approve()

Contract

CapitalPool.sol

Summary

CapitalPool.approve current is using gas consumption thay can be optimised, it does this by always attempting to approve the maximum possible amount.

Vulnerability Details

So the approve function, always attempts to approve the maximum possible amount type(uint256).max for each token, regardless of the current allowance:

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

Impact

Too much gas consumption, when it can be optimized, as if the current allowance is already sufficient or close to the maximum value, especially if the approve function is called frequently or for multiple tokens in a single transaction, too much gas is used.

Proof of Concept

​
function testUnnecessaryApproval() public {
address token = address(1);
// First approval
uint256 gasCost1 = gasleft();
capitalPool.approve(token);
gasCost1 = gasCost1 - gasleft();
​
// Second approval (unnecessary)
uint256 gasCost2 = gasleft();
capitalPool.approve(token);
gasCost2 = gasCost2 - gasleft();
​
// Both approvals should consume similar gas, showing the inefficiency
assertApproxEqAbs(gasCost1, gasCost2, 1000, "Second approval should consume similar gas");
}
​

Recommendations

Check to only increase the allowance if necessary:

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
uint256 currentAllowance = IERC20(tokenAddr).allowance(address(this), tokenManager);
if (currentAllowance < type(uint256).max / 2) {
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
if (!success) {
revert ApproveFailed();
}
}
}

Trigger an approval if the current allowance is less than half the maximum value, optimizing gas consumption.

[L-07] Not effective rounding: OfferLibraries

Contract

OfferLibraries.sol

Summary

OfferLibraries uses wrong rounding methods across different functions, causing small discrepancies in calculations.

Vulnerability Details

Bug is in the getDepositAmount function, that uses a parameter _rounding to determine the rounding direction:

return
Math.mulDiv(
_amount,
_collateralRate,
Constants.COLLATERAL_RATE_DECIMAL_SCALER,
_rounding
);

Because getRefundAmount function uses hardcoded rounding:

uint256 usedAmount = Math.mulDiv(
_amount,
_usedPoints,
_points,
Math.Rounding.Ceil
);

Impact

Varied discrepancies between deposit and refund calculations, which could accumulate over time and cause small but relevant financial drawbacks.

Proof of Concept

​
function testInconsistentRounding() public {
uint256 amount = 100;
uint256 points = 10;
uint256 usedPoints = 3;
uint256 collateralRate = 15000; // 150%
​
uint256 depositAmount = OfferLibraries.getDepositAmount(
OfferType.Ask,
collateralRate,
amount,
true,
Math.Rounding.Up
);
​
uint256 refundAmount = OfferLibraries.getRefundAmount(
OfferType.Ask,
amount,
points,
usedPoints,
collateralRate
);
​
// The rounding directions are inconsistent between these two functions
// which could lead to discrepancies in calculations
assertTrue(depositAmount != refundAmount, "Rounding should be inconsistent");
}

Recommendations

Make the rounding direction consistent across both functions. Either via making the rounding direction a parameter in getRefundAmount or use a consistent rounding direction across both functions:

function getRefundAmount(
OfferType _offerType,
uint256 _amount,
uint256 _points,
uint256 _usedPoints,
uint256 _collateralRate,
Math.Rounding _rounding
) internal pure returns (uint256) {
uint256 usedAmount = Math.mulDiv(
_amount,
_usedPoints,
_points,
_rounding
);
// same
}

[L-08] No upper bound check for platform fee rate: SystemConfig

Contract

SystemConfig.sol

Summary

SystemConfig has no upper bounds constraint for the platform fee rate, allowing the setting of too high fees.

Vulnerability Details

updateUserPlatformFeeRate function, does not check that the platform fee rate doesn't exceed Constants.PLATFORM_FEE_DECIMAL_SCALER, but this allows setting the fee to 100%, which is not intended:

require(
_platformFeeRate <= Constants.PLATFORM_FEE_DECIMAL_SCALER,
"Invalid platform fee rate"
);

Impact

Setting an extremely high platform fee could severely impact user profits.

Proof of Concept

function testExcessivePlatformFee() public {
address user = address(1);
uint256 excessiveFee = 9999; // 99.99%
​
// This should not revert, but it allows for an extremely high platform fee
systemConfig.updateUserPlatformFeeRate(user, excessiveFee);
​
uint256 fee = systemConfig.getPlatformFeeRate(user);
assertTrue(fee > 5000, "Platform fee should be unreasonably high");
}

Recommendations

Insert reasonable upper bound for the platform fee rate:

uint256 constant MAX_PLATFORM_FEE_RATE = 1000; // 10% as possible one
require(
_platformFeeRate <= MAX_PLATFORM_FEE_RATE,
"Platform fee rate too high"
);

Making sure platform fee rate cannot exceed a reasonable maximum, protecting users excessive fees.

[L-09] Need better validation: SystemConfig

Contract

SystemConfig.sol

Summary

SystemConfig has no validation for the settlementPeriod parameter when updating market information.

Vulnerability Details

updateMarket function, there's no validation for the _settlementPeriod parameter:

marketPlaceInfo.settlementPeriod = _settlementPeriod;

An extremely long or short settlement period could be set.

Impact

A bad settlement period could disrupt the market's functioning, either by making settlements occur too frequently or too infrequently, gamng the system.

Proof of Concept

function testInvalidSettlementPeriod() public {
string memory marketPlaceName = "Test";
address tokenAddress = address(1);
uint256 tokenPerPoint = 1e18;
uint256 tge = block.timestamp + 1 days;
uint256 extremelyLongPeriod = 365 days;
​
// This should not revert, but it allows for an extremely long settlement period
systemConfig.updateMarket(marketPlaceName, tokenAddress, tokenPerPoint, tge, extremelyLongPeriod);
​
// In a real scenario, this could disrupt market dynamics
assertTrue(extremelyLongPeriod > 30 days, "Settlement period should be unreasonably long");
}
​

Recommendations

Validation for the settlement period so it falls within good range:

uint256 constant MIN_SETTLEMENT_PERIOD = 1 hours;
uint256 constant MAX_SETTLEMENT_PERIOD = 30 days;
require(
_settlementPeriod >= MIN_SETTLEMENT_PERIOD && _settlementPeriod <= MAX_SETTLEMENT_PERIOD,
"Invalid settlement period"
);

Above makes sure settlement period is set within a god timeframe, maintaining the intended dynamics.

Updates

Lead Judging Commences

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

finding-PreMarkets-Rounding-Direction

Duplicate of #456, however, for issues noting rounding directions, will be low severity given the impact is not proven sufficiently with a PoC/numerical example and most rounding will not result in significant losses e.g. most examples only proved at most a 1 wei difference when computing `depositAmount/platFormFees` and involves lower amount offers

Support

FAQs

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

Give us feedback!