TT Traxx

CertiK26 Jul 2023Files named in the report

Audit

CertiK reviewed Traxx (TT Traxx) on 26 Jul 2023: 40 findings, 1 critical; 23 marked fixed, 5 acknowledged, 12 open.

1 critical finding, fixed

Open the report Open Traxx’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings40

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	1
High	0	4	1
Medium	0	1	6
Low	12	0	15

CriticalImproper Approval For The First OwnerFixed

The vulnerability lies in the `addQuantity()` function, which is intended to mint additional tokens of an existing token type (identified by `id`) to a specified address (`_to`). The basic functionality of this function is to mint a specific number of tokens (`amount`) and assign them to the `_to` address. Additionally, it ensures that only the first owner or an admin can call this function. The critical issue arises when the function sets approval for token management. Specifically in the following lines: ```solidity=377 emit TraxxMinted(msg.sender, _to, id, _uri, amount); if (msg.sender != _to) _setApprovalForAll(_to, msg.sender, true); _setApprovalForAll(_to, market, true); _tokenIdTracker.increment(); ``` The `_setApprovalForAll()` function grants the first owner and the market the ability to manage **all** ERC1155 tokens belonging to the `_to` address. This is a critical issue, as it provides the first owner with unrestricted control over another user's entire collection of ERC1155 tokens.

HighOwnership Not Transferred To the Owner When deploFixed

There is an inconsistency in the admin assignment for proxies in the two deployment functions. In the `deployTransparent()` function, the admin of the proxy is set to the `Factory` contract (through the deployment of a new `ProxyAdmin` contract instance). In contrast, in the `deployUUPS()` function, the admin of the proxy is set to the owner address due to the design of `UUPSProxy` contract. This inconsistency might lead to different levels of control and security for the deployed proxies. For the transparent proxy, the `Factory` contract has control over the proxy, which means the factory can perform upgrades on the proxy contract. In the case of the UUPS proxy, the `owner` has control, allowing them to perform upgrades or other admin actions directly. Therefore, as the owner of the deployed transparent proxy remained to be the factory contract, the owner can never be changed anymore and thus unable to upgrade the implementation contract.

MediumDuplicate Addresses Can Bypass Check of `maxKeysPFixed

The `validPurchaseParameters` function validates purchase parameters in `Lock` contract, but it lacks a check for duplicate addresses in the `_recipients` array. This oversight allows an attacker to bypass the `_balances[_recipients[i]] + _quantities[i] > maxKeysPerAddress` check, potentially leading to unintended consequences. ```solidity=757 for(uint i = 0; i<_recipients.length; ++i){ if( _recipients[i] == address(0) || _quantities[i] == 0 || isTierValid[_tiers[i]] == false || _balances[_recipients[i]] + _quantities[i] > maxKeysPerAddress ){ return false; } } ``` An attacker could exploit this by including **the same address multiple times in the `_recipients` array**, causing the sum of the keys to exceed the `maxKeysPerAddress` limit when the function is called multiple times. This bypasses the balance check and might allow the attacker to perform actions not intended by the smart contract's logic.

MediumEnum Index Error in `deployContract` FunctionFixed

The `deployContract` function in the smart contract has an incorrect `require` statement that checks the contractType value. The first value of an enum is 0 (in this case, `ERC20`), but the function requires `contractType` to be greater than 0. This creates an unnecessary limitation, preventing the deployment of ERC20 contracts using this function. Similarly, the checks for `UPGRADABLE_CONTRACTS_CANNOT_BE_DEPLOYED_HERE` and `NON_UPGRADABLE_CONTRACTS_CANNOT_BE_DEPLOYED_HERE` might be incorrect. ```solidity require(contractType > 0, CONTRACT_TYPE_MUST_BE_VALID); require(contractType <= 6, UPGRADABLE_CONTRACTS_CANNOT_BE_DEPLOYED_HERE); require(contractType != 6, NON_UPGRADABLE_CONTRACTS_CANNOT_BE_DEPLOYED_HERE); ```

MediumInput Validation Missing in `batchMintTraxx()` FuFixed

The `batchMintTraxx()` function is missing input validation for parameters `ids`, `amounts`, `uris`, and `maxPurchaseLimit`. This inconsistency makes it possible for tokens with incorrect or empty `uris` to be minted or for tokens with invalid `amounts` and `maxPurchaseLimit` values to be created. ```solidity=420 require(to != address(0), throwError(ZERO_ADDRESS)); require(ids.length > 0, throwError(INVALID_FUNCTION_PARAMS)); require( (ids.length == amounts.length) && (amounts.length == uris.length) && (uris.length == maxPurchaseLimit.length), throwError(INVALID_FUNCTION_PARAMS) ); _mintBatch(to, ids, amounts, data); uint256 arrayLen = uris.length; for (uint256 i; i < arrayLen; ++i) { _tokenURIs[ids[i]] = uris[i]; _tokenIdTracker.increment(); _primarySalePurchaseLimitPerToken[ids[i]] = maxPurchaseLimit[i]; } ``` In contrast, the `mintTraxx()` function has proper input validation checks for the same parameters. The lack of these checks in the `batchMintTraxx()` function can result in unintended behavior or potential abuse by malicious actors. Moreover, the `batchMintTraxx()` function allows for the overwriting of an existing token's parameters. For instance, the function directly assigns the provided `uris[i]` to the `_tokenURIs[ids[i]]` mapping without checking if the `id` already exists or not. This can lead to unintended consequences and potential confusion among users, especially if the token URI is used for important metadata. Similar issue for the `_primarySalePurchaseLimitPerToken` parameter.

MediumQuantity Constraint Not Enforced for ERC721 TokenFixed

The smart contract code for creating and modifying token listings does not enforce the quantity constraint for ERC721 tokens. The `modifyListing()` functions allow setting a quantity greater than 1 for ERC721 tokens, which can lead to potential issues since ERC721 tokens are non-fungible and should have a quantity of 1. ```solidity=227 Listing memory newListing = Listing({ listingId: listingIdTracker, tokenOwner: tokenOwner, primarySplitAddress: _params.primarySplitAddress, assetContract: _params.assetContract, tokenId: _params.tokenId, quantity: _params.quantityToList, currency: _params.currencyToAccept, buyoutPrice: _params.buyoutPrice, tokenType: listTokenType, listingType: _params.listingType, saleType: saleType }); ``` ```solidity=305 listings[_listingId].buyoutPrice = _params.buyoutPrice; listings[_listingId].quantity = _params.quantityToList; listings[_listingId].primarySplitAddress = _params.primarySplitAddress; ``` Allowing a quantity greater than 1 for ERC721 tokens could cause logical inconsistencies and confusion within the marketplace, as ERC721 tokens are unique and non-fungible. This could potentially lead to unexpected behavior when updating the quantity variable in `buy()` function.

MediumState Variable Initialization In Constructor WithFixed

Code inside the constructor is only executed during the deployment of the contract and only affects the state of the implementation contract. Therefore, it will never be executed in the context of the proxy’s state which makes it irrelevant for an upgradeable contract.

MediumThe `Lock` Contract is Incompatible With EIP721 SFixed

According to the standard of [ERC721](https://eips.ethereum.org/EIPS/eip-721) and the comments in the `Lock` functions, the `transferFrom()` function needs to support if the caller is not `from`, it must be approved to move this token if an authorized operator or the approved address for this NFT. However, the `transferFrom` function is missing the necessary approval check in the `safeTransferFrom` function. Furthermore, due to Line 336, the `transferFrom()` function only supports transferring tokens from the sender to other addresses. Moreover, the standard of ERC721 required `safeTransferFrom(address _from, address _to, uint256 _tokenId)` to work identically to the other function with an extra data parameter, except this function sets data to an empty string. However, it won't call `onERC721Received` on `_to` and throws if the return value is not `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`.

Low`_tokenId < tokenId.current()` Doesn't Mean The `Fixed

In the `getApproved()` function, the condition `_tokenId < tokenId.current()` only checks if the provided `_tokenId` is less than the current maximum tokenId. However, it doesn't account for the possibility of the token being burned, making it invalid. Similar issues in the `TokenTraxx1155` contract.

LowCheck-effect-interaction Pattern ViolationFixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. ### External call(s) ```solidity=370 _mint(_to, id, amount, ""); ``` * This function call executes the following external call(s). * In `ERC1155Upgradeable._doSafeTransferAcceptanceCheck`, * `(response) = IERC1155ReceiverUpgradeable(to).onERC1155Received(operator,from,id,amount,data)` ### State variables written after the call(s) ```solidity=379 _tokenIdTracker.increment(); ``` * This function call executes the following assignment(s). * In `CountersUpgradeable.increment`, * `counter._value += 1`

Show 10 more

LowHardcoded Initializing FunctionFixed

The initializing function might not always be the default `initialize()` function in the implementation contract. In the `deployTransparent()` function, the initializing function is hardcoded as `initialize()`. ```solidity=402 proxyConstructorArgs = abi.encode( implementationAddress, proxyAdminAddress, abi.encodeWithSignature("initialize()") ); ```

LowIncomplete Security Checks in `splitSignature()`Fixed

According to the official Solidity [documentation](https://docs.soliditylang.org/en/v0.8.18/solidity-by-example.html#id3), the function `splitSignature()` does not use all security checks. A real implementation should use a more rigorously tested library, such as openzepplin’s version of this code.

LowInconsistent Comment and CodeFixed

The `isTierValid` mapping does not store the valid Traxx prices, but instead, it stores a boolean value indicating whether a tier is valid or not, which is inconsistent with the comment. Moreover, since `tierDetails` uses `uint16` as its key to indicate a tier's detail, it would make more sense for `tier` in the `isTierValid` struct to also be of type `uint16`, to match the key type of `tierDetails`.

LowInconsistent Native Token CheckFixed

The smart contract has an inconsistency in the definition of native tokens between the `_isNativeToken()` function and the `transferCurrency()` function. In the `_isNativeToken()` function, an address is considered as a native token if it is equal to `NATIVE_TOKEN` or `address(0)`. ```solidity=1102 function _isNativeToken(address _toCheck) internal pure returns (bool) { return _toCheck == NATIVE_TOKEN || _toCheck == address(0); } ``` However, in the `transferCurrency()` function, the native token is identified solely by comparing `_currency` to `NATIVE_TOKEN`. ```solidity=731 if (_currency == NATIVE_TOKEN) { ... ``` This inconsistency could lead to unintended behavior and potential vulnerabilities if other contracts rely on these functions for native token identification and handling.

LowInconsistent With Comment (No Need to Increase `_Fixed

In the `addQuantity()` function, there is no need to increment the `_tokenIdTracker` since it is only meant to mint more tokens of an existing token ID. Incrementing the `_tokenIdTracker` would create a new token ID.

LowMissing `primarySplitAddress` Update in `ListingMFixed

The `modifyListing` function in the contract allows updating the listing's `primarySplitAddress`, but the updated address is not included in the emitted `ListingModified` event. This issue can lead to confusion and potential misinterpretation of events when relying on event logs for tracking changes in listings.

LowMissing Zero Check for `_tiers` in `addNewTiers(Fixed

The `lock` contract has a missing zero check for the `_tiers` input array in the `addNewTiers()` function, which is inconsistent with the `initialize()` function and the `_tiers[i] == _tiersArr[j]` condition in the `supplyAvailability()` function. This missing check could allow adding new tiers with a zero value, which might cause unexpected results when interacting with other functions that assume no zero-value tiers exist.

LowNo Upper Limit in `setTraxxFees()` FunctionFixed

There are no upper boundaries for `setTraxxFees()` function which is used to set the Traxx token fees. Setting the total fee rate up to any arbitrary amount is possible.

LowPotential Damage Due to Unprotected InitializerFixed

One or more logic contracts do not protect their initializers. An attacker can call the initializer and assume ownership of the logic contract, whereby she can perform privilelged operations that either indirectly break the proxy by destroying the logic contract or trick unsuspecting users into believing that she is the owner of the upgradeable contract. ```solidity=27 contract TraxxFactory is ``` - `TraxxFactory` is an upgradeable contract that does not protect its initializer. ```solidity=184 function initialize(address initialAdmin, address initialMaintainer) ``` - `initialize` is an unprotected initializer function. ```solidity=67 function _upgradeToAndCall( ``` - `_upgradeToAndCall` could potentially damage the contract. ```solidity=83 function _upgradeToAndCallUUPS( ``` - `_upgradeToAndCallUUPS` could potentially damage the contract. ```solidity=166 function _upgradeBeaconToAndCall( ``` - `_upgradeBeaconToAndCall` could potentially damage the contract. ```solidity=184 function _functionDelegateCall(address target, bytes memory data) private returns (bytes memory) { ``` - `_functionDelegateCall` could potentially damage the contract.

LowPotential Unintended Revert In Other Third-partyFixed

The `isApprovedForAll()` function has an access control check that restricts the usage of the function to the `_owner` only. This may cause unintended reverts if third-party contracts or operators want to check the state of `_operatorApprovals[_owner][_operator]`. ```solidity if( _owner != msg.sender ){ revert INVALID_SENDER(); } ```

Show 3 more

LowRedundant Balance Check in Custom `_beforeTokenTrFixed

The custom implementation of the `_beforeTokenTransfer()` function in the provided smart contract includes a redundant check for `balanceOf(from, tokenId) >= amount`. This check is already performed in OpenZeppelin's _safeTransferFrom() function after calling the `_beforeTokenTransfer()` hook. ```solidity function _safeTransferFrom( address from, address to, uint256 id, uint256 amount, bytes memory data ) internal virtual { require(to != address(0), "ERC1155: transfer to the zero address"); address operator = _msgSender(); uint256[] memory ids = _asSingletonArray(id); uint256[] memory amounts = _asSingletonArray(amount); _beforeTokenTransfer(operator, from, to, ids, amounts, data); uint256 fromBalance = _balances[id][from]; require(fromBalance >= amount, "ERC1155: insufficient balance for transfer"); unchecked { _balances[id][from] = fromBalance - amount; } _balances[id][to] += amount; emit TransferSingle(operator, from, to, id, amount); _afterTokenTransfer(operator, from, to, ids, amounts, data); _doSafeTransferAcceptanceCheck(operator, from, to, id, amount, data); } ```

LowUnsafe Integer CastFixed

```solidity=893 tierDetails[_tier].duration == -1 ? expirationTimestamp = 0 : expirationTimestamp = block.timestamp + uint(tierDetails[_tier].duration); ``` - The type conversion `uint256(tierDetails[_tier].duration)` from type int256 to type uint256 may flip the value's sign.

LowUser-Defined GettersFixed

The linked functions are equivalent to the compiler-generated getter functions for the respective variables.