Traxx security record

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.

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.

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.

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); ```

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.

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.

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.

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)"))`.

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.

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`