# MetaPayment & Market Reaudit # Audit Details ## Scope of the audit - [MetaPayment](https://github.com/Meta-Force-Space/MetaPayment/tree/1396c56b19c21a49a650c275d89472ec54458dd4/contracts) - [Market](https://github.com/Meta-Force-Space/market/tree/3b4bb6ed2a58bc311ad097aef54f1d6928c91ceb/contracts) ## Severity Criteria - Critical: Bugs leading to assets theft, fund access locking, or any other loss of funds. - High: Bugs that break contract core logic. - Medium: Bugs leading to partial failure of the contract minor logic under specific conditions. - Informational: Bugs, suggestions, optimizations that do not have a significant immediate impact. # Critical ## Double-spend vulnerability in `Auction` contract ### Description The `Auction` contract exhibits a critical flaw in its token handling logic, leading to potential double-spending for ERC1155 tokens and bid blocking for ERC721 tokens. The vulnerability arises due to redundant token transfer calls. For ERC721: ```solidity function onERC721Received(...) external override returns (bytes4) { ... IERC721(msg.sender).safeTransferFrom( address(this), address(_exchangeGate), tokenId, "" ); // <-- Token is transferred here ... if (action == Action.Bid) { ... _makeBid(lotId, msg.sender, tokens); // <-- Bid logic tries to make another transfer } ... } function _makeBid(...) internal { ... _receiveTokens(tokens, user); // <-- Another transfer leading to revert ... } ``` For ERC1155: ```solidity function onERC1155Received(...) external override returns (bytes4) { Tokens memory tokens = Tokens({ tokenType: TokenType.Erc1155, tokenAddress: msg.sender, id: id, count: value }); _receiveTokens(tokens, address(this)); // <-- token transfered to Exchange Gate here ... if (action == Action.Bid) { ... _makeBid(lotId, msg.sender, tokens); // <-- Bid processing with token transfer } ... } function _makeBid(...) internal { ... _receiveTokens(tokens, user); // <-- Another transfer leading to revert or double spend of tokens ... } ``` In both cases, the issue is the additional call to `_receiveTokens` within `_makeBid`, which performs an unnecessary token transfer after the initial transfer in the `onERC1155Received` and `onERC721Received` functions. In case of ERC721 token it will lead to revert, but in case of ERC1155 it will lead to both revert or double spend tokens if there were some on contract balance. ### Recommendation It is recommended to make call of `_receiveTokens` within `_makeBid` only in case of ERC-20 tokens. ## Claiming issue with non-ERC20 tokens in `Auction` contract ### Description The `Auction` contract contains a logic error that prevents the successful claiming of a lot subject when purchased with ERC721 or ERC1155 tokens. The issue arises due to the `ExchangeGate.exchange` function's limitation to only support ERC20 tokens as buyer tokens. ```solidity function _claim(uint256 lotId) internal { Lot storage lot = _lots[lotId]; ... _exchangeGate.exchange( lot.owner, subject, lot.bid.user, lot.bid.tokens, lot.commission, true, true ); // <-- Fails for non-ERC20 buyer tokens ... } function exchange(...) public onlyRole(CONTRACT_ROLE) { ... require(buyerTokens.tokenType == TokenType.Erc20); // <-- ERC20 restriction ... } ``` The `exchange` function in `ExchangeGate` is designed to only allow transactions where the buyer uses ERC20 tokens. This condition leads to a revert in transactions involving ERC721 or ERC1155 tokens, resulting in the tokens and lot subject being locked in the `ExchangeGate`, with no way for the bid winner or lot owner to withdraw them. ### Recommendation It is recommended to update the `ExchangeGate.exchange` function to support transactions involving ERC721 and ERC1155 tokens as buyer tokens. # High ## Reentrancy Vulnerability in Auction Contract's Bid Function ### Description The `Auction` contract has a reentrancy vulnerability in its `_makeBid` function. This vulnerability arises during the handling of bids where the `prevBidUser` can potentially reenter the contract and overbid the current `user` within the same transaction. The risk occurs in the sequence of actions taken when a bid is placed that meets the `instasellPrice`, triggering an immediate claim. ```solidity function _makeBid(uint256 lotId, address user, Tokens memory tokens) internal { ... address prevBidUser = lot.bid.user; lot.bid.user = address(0); if (prevBidUser != address(0)) { _exchangeGate.withdraw(lot.bid.tokens, prevBidUser); // <-- Potential for reentrancy } _receiveTokens(tokens, user); ... if (lot.bid.tokens.count >= lot.parameters.instasellPrice) { ... _claim(lotId); // <-- immediate claim } } ``` The vulnerability exists because the call to `_exchangeGate.withdraw` for the `prevBidUser` can contain callbacks in the case of ERC-721 and ERC-1155 tokens, which could be exploited to reenter the `_makeBid` function. If this happens, the `prevBidUser` could place a new bid before the current transaction completes, causing the `user` to lose their funds. ### Recommendation It is recommended to add reentrancy guard for every function that calls `_makeBid`. # Medium ## Non-standard ERC20 token handling ### Description The `_transferErc20` function in the `Auction` contract uses a direct `require` statement to ensure the success of ERC20 token transfers, instead of utilizing the `safeTransfer` method from the imported library. This approach may lead to issues with ERC20 tokens that do not return a boolean value on transfers, causing the function to revert unintentionally. The issue arises because not all ERC20 tokens adhere to the standard of returning a boolean value upon the execution of `transfer` or `transferFrom` (check the [link](https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca)). In cases where the ERC20 token does not return any value, the `require` statement will fail, and the transaction will revert, even if the transfer itself was successful. ### Recommendation It is recommended to utilize the `safeTransfer` and `safeTransferFrom` methods provided by the OpenZeppelin's SafeERC20 library. ## Lack of zero address check in `MetaPayment.getPaymentAddressForId` ### Description The `getPaymentAddressForId` function in the `MetaPayment` contract lacks a check to ensure that the returned address is not a zero address. This can lead to a scenario where funds are inadvertently transferred to the zero address, resulting in a permanent loss of those funds. ```solidity function getPaymentAddressForId(uint256 id) public view returns (address) { address payment = getReservedAddress[id]; if (payment == address(0)) { payment = metaCore.getUserAddress(id); // ^-- No check to ensure 'payment' is not zero after this assignment } else { ... } return payment; } ``` In the code, if the `getReservedAddress[id]` is zero, the function fetches an address from `metaCore.getUserAddress(id)`. However, there is no subsequent check to validate that this fetched address is non-zero. ### Recommendation It is recommended to add a sanity check after the `metaCore.getUserAddress(id)` call to ensure that the `payment` variable is not set to the zero address. # Informational ## Inconsistency in role definitions ### Description The `MetaPayment` contract have inconsistent styles in defining contract roles, which can potentially lead to confusion and collision issues. The roles `META_ROLE` and `RESERVED_ADDRESS_SETTER_ROLE` are defined using different approaches. ```solidity bytes32 constant META_ROLE = 0xa9ce9b5120c53e0d8b4fe6e0814a47efbead9ebc4d29ec54571f37f0b61ecf0f; // Hardcoded hash bytes32 constant RESERVED_ADDRESS_SETTER_ROLE = keccak256("RESERVED_ADDRESS_SETTER_ROLE"); // Hashed from string ``` ### Recommendation To ensure clarity and avoid potential role collisions, it is recommended to unify the role definition style. ## Unused function `transferDirect` ### Description The function `transferDirect` in the `MetaPayment` contract is never unused in the code. ### Recommendation It is recommended to remove redundant function.