Talentir Token & Marketplace security record

A malicious seller can submit a sell order which causes a revert on any subsequent buy transaction. The attack is made possible by two components. Firstly, the addition of a very cheap sell order guarantees its execution the next time a user submits a buy order. Secondly, when the seller receives ETH, the fallback function in their contract can be used to expend the transaction's gas such that it does not have enough to complete the order. The `TalentirMarketplaceV1` allows users to either buy Talentir tokens with the chain's native currency, such as ETH or sell Talentir tokens in exchange for the chain's native currency. If a user submits a buy order, `TalentirMarketplaceV1` first tries to find the cheapest sell order to fill the buy order with. Thus, if a malicious seller is able to submit a sell order with the lowest price, this order is always guaranteed to be used to fill the next order. This can be seen in the function which is used to retrieve the appropriate sell order: ``` function getBestOrder(uint256 _tokenId, Side _side) public view returns (uint256, uint256) { uint256 price = _side == Side.BUY ? markets[_tokenId][_side].priceTree.last() : markets[_tokenId][_side].priceTree.first(); uint256 bestOrderId; (, bestOrderId, ) = markets[_tokenId][_side].orderList[price].getNode(0); return (bestOrderId, price); } ``` When the order actually executes, ETH (or another native coin) is sent to the seller through a low-level `call()`. According to [EIP-150](https://eips.ethereum.org/EIPS/eip-150), by default `63/64` of the remaining gas can be used by default in a low-level call. Several actions happen after this call, including the emission of an event and ETH transfers to other addresses. If the `call()` to the seller expends too much gas, then these other actions will fail as there is not enough gas left to execute them: ``` function _executeOrder( address _sender, uint256 _orderId, uint256 _quantity ) internal returns (uint256 ethQuantity) { ... (locals.success, ) = locals.seller.call{value: locals.payToSeller}(""); (locals.success, ) = locals.royaltiesReceiver.call{value: locals.royalties}(""); (locals.success, ) = talentirFeeWallet.call{value: locals.talentirFee}(""); emit OrderExecuted( ... } ``` By placing a very cheap sell order which would invoke an expensive fallback function, the malicious seller is able to stop the marketplace from functioning for that `tokenId`. All subsequent buy orders would revert. Subsequent sell orders would not revert but would never get executed due to the buy orders reverting. Note that the same exploit is also possible to perform on the buy side, by offering a high price and then implementing a malicious ERC-1155 receiver function.

The function `TalentirMarketplaceV1._addOrder()` only ever appends new orders (in increasing order ID number) to the front of the linked list `markets[_tokenId][_side].orderList[_price]`. ``` markets[_tokenId][_side].orderList[_price].push(nextOrderId, true); ``` However, the function `TalentirMarketplaceV1._removeOrder()` always removes the last element/order from the linked list `markets[tokenId][side].orderList[price]` in `L378`, without confirming it being the to-be-deleted element: ``` markets[tokenId][side].orderList[price].pop(false); ``` This could lead to the loss of order IDs other than the desired one. While other "copies" of such deleted order IDs exist (`orders` and `userOrders`), the function `getBestOrder()` always takes the first inserted element of that linked list, which in turn is used when filling existing orders when creating a new one. Deleting and reading order IDs this way would lead to a deteriorated order book, where orders of other users may be (maliciously) removed from the order book.

The `mint()` function makes sure that the `contentID` converted to `tokenId` is unique. However, the IPFS `cid` can be duplicated, which would allow multiple `tokenId`s to have the same URI.

The `TalentirMarketplaceV1` inherits from `ERC1155Holder`, which unconditionally accepts any ERC-1155 tokens as shown below; ``` contract ERC1155Holder is ERC1155Receiver { function onERC1155Received( address, address, uint256, uint256, bytes memory ) public virtual override returns (bytes4) { return this.onERC1155Received.selector; } function onERC1155BatchReceived( address, address, uint256[] memory, uint256[] memory, bytes memory ) public virtual override returns (bytes4) { return this.onERC1155BatchReceived.selector; } } ``` This can result in non-Talentir ERC-1155 tokens becoming stuck in the contract. It may also result in Talentir tokens being locked in the marketplace contract if they are not transferred only through the use of the `makeSellOrder()` function.

The computation of the talentir fee amount for a given total price as computed in `TalentirMarketplaceV1.calcTalentirFee()` unnecessarily multiplies and then at the end divides by `100`: ``` return ((100 * talentirFeePercent * _totalPaid) / PERCENT) / 100; ``` As the contract is using solidity version 0.8.17, which implicitly performs underflow/overflow checks on arithmetic operations, providing a total price higher than ``` 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff / (100 * 100_000) ``` which is equal to: ``` 0x10c6f7a0b5ed8d36b4c7f34938583621fafc8b0079a2834d26fa3fcc9ea9 ``` will lead to overflows and revert the operations and thereby preventing the execution on such orders.

The `TalentirMarketplaceV1` contract divides the amount of ETH by the amount of Talentir tokens to sell or buy to store the price. However, this will result in an immediate loss of precision in the price and an indirect loss of precision in subsequent calculations. The loss of precision will at the very least result in sellers' getting less ETH (or other native currency) than they should. Regardless of whether a user places a sell or a buy order, the price is always calculated using the following code: ``` unction _makeOrder( address _sender, uint256 _tokenId, Side _side, uint256 _ethQuantity, uint256 _tokenQuantity, bool _addOrderForRemaining ) internal { ... uint256 price = _ethQuantity / _tokenQuantity; ... } ``` This price is then used to determine which orders are suitable for filling orders coming in on the opposite side. The loss of precision in the price calculation can result in an unfavorable result for the Talentir token seller, as shown in the exploit scenario.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transitions can be logged, which is beneficial for monitoring the contract, and also tracking eventual bugs or hacks. Below we present a non-exhaustive list of events that could be emitted to improve application management. 1. `TalentirTokenV1` 1. Emit an event from `setMinterRole()`. 2. Emit an event from `setMarketplace()`. 1. `TalentirMarketplaceV1` 1. `_removeOrder()`: Further information should be included in this event such as `_orderId` `orders[_orderId].sender`, `price`, `side` and `tokenId`.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. We have listed all missing validations below: 1. `TalentirTokenV1` 1. `updateTalent()` 1. Validate that `talent` is not the zero address. 2. `mint()` 1. Validate that `talent` is not the zero address. 2. Validate that `cid` and `contentID` are not empty strings. 3. `setGlobalPresaleAllowance()`: Validate that `user` is not the zero address. 4. `setTokenPresaleAllowance()`: 1. Validate that `user` is not the zero address. 2. Validate that `tokenId` already exists. 5. `setMinterRole()`: Validate that `minterAddress` is not the zero address. 6. `setMarketplace()`: Validate that `marketplace` is not the zero address. 2. `TalentirMarketplaceV1` 1. `constructor()`: Validate that `_talentirNFT` is not the zero address. 2. `setTalentirFee()`: Validate that `_wallet` is not the zero address. 3. `_makeOrder()`: Validate that the `_tokenQuantity` is greater than the maximum number of fractions a token can have, i.e. `1_000_000`.

Flase positive removed from report.

Most functions will return a `True` or `False` value upon success. Some functions, like `send()`, are more crucial to check than others. It is important to ensure that every necessary function is checked. In particular, consider the following instances: 1. `TalentirMarketplaceV1.sol#L186`in function `cancelOrders()`. 1. `TalentirMarketplaceV1.sol#L279`in the function `_makeOrder()`. 1. `TalentirMarketplaceV1.sol#L323`in the function `_executeOrder()`. 1. `TalentirMarketplaceV1.sol#L324`in the function `_executeOrder()`. 1. `TalentirMarketplaceV1.sol#L325`in the function `_executeOrder()`.