Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

PreMarkets.sol 2 critical issues

arhan

Vulnarabilities:

1-CWE-1164 (SWC-131) - Presence of unused variables

Unused variables are allowed in Solidity and they do not pose a direct security issue. It is best practice though to avoid them as they can: cause an increase in computations (and unnecessary gas consumption) indicate bugs or malformed data structures and they are generally a sign of poor code quality cause code noise and decrease readability of the code

2-CWE-480 (SWC-129) - Typographical Error

A typographical error can occur for example when the intent of a defined operation is to sum a number to a variable (+=) but it has accidentally been defined in a wrong way (=+), introducing a typo which happens to be a valid operator. Instead of calculating the sum it initializes the variable again. The unary + operator is deprecated in new solidity compiler versions.

Security Issues:

1. Missing Checks on External Calls and Reentrancy Guard

Issue

The contract makes multiple external calls (e.g., to tadleFactory.getSystemConfig() and tokenManager.tillIn{value: msg.value}()) without reentrancy protection. This could lead to reentrancy vulnerabilities, where an attacker calls back into the contract before the first function invocation fully completes.

Fix

Use the ReentrancyGuard contract from OpenZeppelin to prevent reentrancy attacks. Applying the nonReentrant modifier to functions that make external calls can mitigate the risk of reentrancy attacks.

Updated Code Example

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract PreMarkets is PerMarketsStorage, Rescuable, Related, IPerMarkets, ReentrancyGuard {
// Constructor stays the same
constructor() Rescuable() {}
​
function createOffer(CreateOfferParams calldata params) external payable nonReentrant {
// Function body remains the same
}
function createTaker(address _offer, uint256 _points) external payable nonReentrant {
// Function body remains the same
}
function listOffer(address _stock, uint256 _amount, uint256 _collateralRate) external payable nonReentrant {
// Function body remains the same
}
// Apply nonReentrant to other functions making external calls
}

2. Unchecked Address Generation

Issue

Addresses for maker, offer, and stock are generated without ensuring that they are unique or valid, and the generated address could potentially be manipulated.

Fix

Use keccak256 for deterministic address generation and confirm uniqueness by checking against relevant mappings.

Updated Code Examples

function _generateAddress(bytes32 salt) internal view returns (address) {
return address(uint160(uint256(keccak256(abi.encodePacked(address(this), salt)))));
}
​
function uniqueGenerateAddress(uint256 id) internal view returns (address) {
address newAddr = _generateAddress(keccak256(abi.encodePacked(id)));
require(makerInfoMap[newAddr].authority == address(0), "Address already exists");
return newAddr;
}
​
function createOffer(CreateOfferParams calldata params) external payable nonReentrant {
// ... previous code remains
​
address makerAddr = uniqueGenerateAddress(offerId);
address offerAddr = uniqueGenerateAddress(offerId);
address stockAddr = uniqueGenerateAddress(offerId);
​
// Continue as normal
}

3. Lack of Access Control on Critical Functions

Issue

Functions such as updateOfferStatus and updateStockStatus can be called by anyone if the onlyDeliveryPlace modifier is compromised. Additionally, onlyDeliveryPlace modifier seems custom and should be analyzed for potential risks.

Fix

Explicitly verify the caller using standardized access control patterns, such as OpenZeppelin's AccessControl.

Updated Code Example

import "@openzeppelin/contracts/access/AccessControl.sol";
​
contract PreMarkets is PerMarketsStorage, Rescuable, Related, IPerMarkets, ReentrancyGuard, AccessControl {
bytes32 public constant DELIVERY_ROLE = keccak256("DELIVERY_ROLE");
​
constructor() Rescuable() {
_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
}
​
function updateOfferStatus(address _offer, OfferStatus _status) external onlyRole(DELIVERY_ROLE) {
OfferInfo storage offerInfo = offerInfoMap[_offer];
offerInfo.offerStatus = _status;
emit OfferStatusUpdated(_offer, _status);
}
​
function updateStockStatus(address _stock, StockStatus _status) external onlyRole(DELIVERY_ROLE){
StockInfo storage stockInfo = stockInfoMap[_stock];
stockInfo.stockStatus = _status;
emit StockStatusUpdated(_stock, _status);
}
​
// Restrict role assignments for enhanced security
function grantDeliveryRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) {
grantRole(DELIVERY_ROLE, account);
}
​
function revokeDeliveryRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) {
revokeRole(DELIVERY_ROLE, account);
}
}

4. Inadequate Input Validation

Issue

Inputs such as amounts and addresses are not always validated properly, allowing the possibility of passing in invalid values.

Fix

Implement comprehensive checks for input validation to ensure the input values are within allowed ranges and are not zero or invalid.

Updated Code Example

function createOffer(CreateOfferParams calldata params) external payable nonReentrant {
require(params.points > 0, "Points must be greater than zero");
require(params.amount > 0, "Amount must be greater than zero");
require(params.eachTradeTax <= Constants.EACH_TRADE_TAX_DECIMAL_SCALER, "Invalid eachTradeTax");
require(params.collateralRate >= Constants.COLLATERAL_RATE_DECIMAL_SCALER, "Invalid collateralRate");
require(params.marketPlace != address(0), "Invalid marketplace address");
require(params.tokenAddress != address(0), "Invalid token address");
​
// Continue as normal
}

5. Hard-Coded Constants and Magic Numbers

Issue

Magic numbers and hardcoded values in smart contracts can obscure the intended behavior and make maintenance difficult. They also introduce risk for errors when values need changes.

Fix

Extract magic numbers and hardcoded values into constants or configuration variables.

Updated Code Example

uint256 constant INVALID_POINTS = 0;
uint256 constant INVALID_AMOUNT = 0;
​
function createOffer(CreateOfferParams calldata params) external payable nonReentrant {
require(params.points != INVALID_POINTS, "Points must be greater than zero");
require(params.amount != INVALID_AMOUNT, "Amount must be greater than zero");
​
// Continue as before
}

6. Missing Events on Critical State Changes

Issue

Important state changes should emit events to provide transparency and aid in monitoring and troubleshooting.

Fix

Ensure that an event is emitted in all critical state-changing functions not already emitting one.

Updated Code Example

event OfferClosed(address indexed offer, address indexed caller);
event OfferRelisted(address indexed offer, address indexed caller);
​
function closeOffer(address _stock, address _offer) external {
// ... existing logic
emit OfferClosed(_offer, _msgSender());
}
​
function relistOffer(address _stock, address _offer) external payable nonReentrant {
// ... existing logic
emit OfferRelisted(_offer, _msgSender());
}
Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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