bazaar.art Marketplace - Haqq

In the Listing contract, users can send ether to the buyFixedSale payable function, in order to purchase a NFT listed for a fixed price (in ether). The protocol commission, when considered, is applied on top of the listing (selling) value, and needs to be added-up to the msg.value, otherwise the call to the function will revert.

    function buyFixedSale(uint256 saleListingID) external payable existingFixedSaleListing(saleListingID) nonReentrant {

In this case, users can send more than required ether to perform the purchase transaction (considering protocol commissions). The contract doesn't have a function that allows authorized parties and/or protocol administrators to withdraw any remaining ether from the contract, what can lead to funds being locked-up in the contract permanently.

The following forge test demonstrates that it is possible to send arbitrary amounts of ether to the Listing contract, having the remaining funds locked.

Test case:

function test_purchase_fixed_sale_erc721h() public {
        // build the state of one NFT listed by Morpheus
        test_listing_list_fixed_sale_erc721h();

        // check ETH balances before
        uint256 niobe_native_balance_before = niobe.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Niobe native ETH balance before purchase:"),
            niobe_native_balance_before
        );

        uint256 morpheus_native_balance_before = morpheus.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Morpheus native ETH balance before purchase:"),
            morpheus_native_balance_before
        );

        uint256 listing_contract_native_balance_before = listing_address.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Listing contract native ETH balance before purchase:"),
            listing_contract_native_balance_before
        );

        uint256 comission_address_native_balance_before = commission_address.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Comission address native ETH balance before purchase:"),
            comission_address_native_balance_before
        );

        // Niobe wants to buy the NFT
        vm.startPrank(niobe);

        uint8 _listing_id = 1;
        uint256 _nft_price = 7 ether;

        // call `buyFixedSale` function with value and listing ID + ether surplus
        (bool success, ) = address(listing).call{value: _nft_price + 2 ether}(abi.encodeWithSignature("buyFixedSale(uint256)", _listing_id));
        require(success, "Call Reverted");

        vm.stopPrank();

        // check ETH balances after
        uint256 niobe_native_balance_after = niobe.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Niobe native ETH balance after purchase:"),
            niobe_native_balance_after
        );

        uint256 morpheus_native_balance_after = morpheus.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Morpheus native ETH balance after purchase:"),
            morpheus_native_balance_after
        );

        uint256 listing_contract_native_balance_after = listing_address.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Listing contract native ETH balance after purchase:"),
            listing_contract_native_balance_after
        );

        uint256 comission_address_native_balance_after = commission_address.balance;
        console.log(
            StdStyle.green("\n\n[BALANCE] Comission address native ETH balance after purchase:"),
            comission_address_native_balance_after
        );

    }

Stack trace:

    ├─ [104081] 0x71eACd69f581665CC7F511dF9670Ef61f335Bc91::buyFixedSale{value: 9000000000000000000}(1)
    │   ├─ [1048] 0xB242b8DdCEcEA9bdf7B38fBd2C828dfc8ef17c80::isWhitelisted(0x57369153bB16A0E736FE95AE3A19a76Add02D70A, 1) [staticcall]
    │   │   └─ ← true
    │   ├─ [639] 0x57369153bB16A0E736FE95AE3A19a76Add02D70A::supportsInterface(0x01ffc9a700000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ [639] 0x57369153bB16A0E736FE95AE3A19a76Add02D70A::supportsInterface(0xffffffff00000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← false
    │   ├─ [552] 0x57369153bB16A0E736FE95AE3A19a76Add02D70A::supportsInterface(0x80ac58cd00000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ [29928] 0x57369153bB16A0E736FE95AE3A19a76Add02D70A::safeTransferFrom(0x033aee9f29811662CE15fe38565320d5D455AFE5, 0x7f61DEdEf4Ea255E8FAfbE3e34857ea063F34324, 1, 0x)
    │   │   ├─ emit Transfer(from: 0x033aee9f29811662CE15fe38565320d5D455AFE5, to: 0x7f61DEdEf4Ea255E8FAfbE3e34857ea063F34324, tokenId: 1)
    │   │   └─ ← ()
    │   ├─ [693] 0x57369153bB16A0E736FE95AE3A19a76Add02D70A::royalties(1) [staticcall]
    │   │   └─ ← 0x033aee9f29811662CE15fe38565320d5D455AFE5, 10
    │   ├─ [0] 0x033aee9f29811662CE15fe38565320d5D455AFE5::fallback{value: 700000000000000000}()
    │   │   └─ ← ()
    │   ├─ [0] console::log("Sent royalties to NFT Creator") [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log("NFT Creator (minter) address:") [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(0x033aee9f29811662CE15fe38565320d5D455AFE5) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] 0x033aee9f29811662CE15fe38565320d5D455AFE5::fallback{value: 6300000000000000000}()
    │   │   └─ ← ()
    │   ├─ [0] 0xb17075c9D3564628a7054Dd6ee9e4806C275AF61::fallback{value: 700000000000000000}()
    │   │   └─ ← ()
    │   ├─ emit BoughtFixedSale(nftAddress: 0x57369153bB16A0E736FE95AE3A19a76Add02D70A, nftID: 1, buyer: 0x7f61DEdEf4Ea255E8FAfbE3e34857ea063F34324, quantity: 1, saleListingID: 1)
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [0] console::log("\u{1b}[92m\n\n[BALANCE] Niobe native ETH balance after purchase:\u{1b}[0m", 5000000000000000000 [5e18]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log("\u{1b}[92m\n\n[BALANCE] Morpheus native ETH balance after purchase:\u{1b}[0m", 107000000000000000000 [1.07e20]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log("\u{1b}[92m\n\n[BALANCE] Listing contract native ETH balance after purchase:\u{1b}[0m", 1300000000000000000 [1.3e18]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log("\u{1b}[92m\n\n[BALANCE] Comission address native ETH balance after purchase:\u{1b}[0m", 700000000000000000 [7e17]) [staticcall]
    │   └─ ← ()
    └─ ← ()

It is recommended to create a withdraw mechanism with access control, allowing authorized parties and/or administrators to perform safe withdrawals from any remaining ether existent in the contract.

Furthermore, it is recommended to enhance the checks (validations) when this function is called, reverting when the msg.value is higher than the listed price + protocol commission.

Remediation Plan

SOLVED : The Haqq team solved this issue in the commit id 1d6c5a0913e01ac2400d574822b9da169a79d50b.

The contracts in-scope are safe-guarded by an access control mechanism, which is a good practice in smart contract development and prevents non-authorized parties and bad actors to perform unauthorized activities within the contracts and ecosystem.

Owners with privileged rights to perform administrative operations need to be trusted and have power mitigated to prevent unilateral actions.

In Wallet contract, updateBalances function, gives the ability to the protocol owner to transfer locked (deposited) amounts from user wallet to protocol wallets.

    function updateBalance(uint256 mintingID, uint256 gasFee, bool unlock) external override onlyAuthorized {
        uint256 amount = balances[mintingID].locked;
        balances[mintingID].locked = 0;
        if (amount > 0) {
            if (!unlock) {
                (bool successDAO, ) = payable(daoAddress).call{value: amount}("");
                require(successDAO, "W: Failed to pay daoAddress");
            } else if (gasFee > 0) {
                require(amount >= gasFee, "W: User balance is lower than gas fee");
                balances[mintingID].available = amount - gasFee;
                plateformBalance += gasFee;
            }
        }
    }

In Listing contract, the owner has privileged rights in unlistFixedSale function.

    function unlistFixedSale(uint256 saleListingID) external existingFixedSaleListing(saleListingID) {

{ ...}

        require(msg.sender == nftOwner || msg.sender == owner(), "L: not the nftOwner or contract owner");

While centralization doesn't directly pose a security concern, it is always a security best practice, mitigating the risk of critical operations being performed by a single actor.

It is recommended to use a multi-signature mechanism (multisign) and establish a secure minimum signatures' threshold for triggering access-controlled calls to smart contracts.

Remediation Plan

RISK ACCEPTED : The Haqq team accepted the risk of this issue. The risk is currently mitigated through the usage of multi-signature wallets.

The Ownable2Step pattern is a variant of the traditional Ownable pattern in smart contract design, specifically aimed at adding another layer of security for ownership transfers.

In the standard Ownable pattern, ownership of the contract can be transferred with a single transaction, which, while efficient, poses a security risk if the owner's account is compromised or in cases of accidental transfers to incorrect addresses.

The Ownable2Step pattern mitigates this risk by introducing a two-step process for ownership transfer. The current owner initiates the transfer by proposing a new owner, but the transfer only completes when the proposed new owner accepts it.

This process ensures that ownership can only be transferred to an address that consents to take over the responsibilities, thereby reducing the risk of accidental or malicious transfers.

Implement the official Ownable2Step OpenZeppelin's implementation by inheriting from Ownable2Step instead of Ownable.

• You can find the source code in the following GitHub link: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

Remediation Plan

ACKNOWLEDGED: The Haqq team acknowledged this issue.

Outdated compiler version:

It was identified an outdated compiler version in multiple contracts

As from the official Solidity documentation, it is recommended to use the latest released version of Solidity.

It was identified that the contracts in the set under analysis are using solc version 0.8.19, hence, outdated, considering the current Solidity compiler (solc) version is 0.8.24.

Floating pragma:

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

- contracts/Auction.sol [Line: 2](contracts/Auction.sol#L2)

	pragma solidity ^0.8.19;

- contracts/Listing.sol [Line: 2](contracts/Listing.sol#L2)

	pragma solidity ^0.8.19;

- contracts/NFTRegistry.sol [Line: 2](contracts/NFTRegistry.sol#L2)

	pragma solidity ^0.8.19;

- contracts/Wallet.sol [Line: 2](contracts/Wallet.sol#L2)

	pragma solidity ^0.8.19;

- contracts/libraries/NFTIdentifier.sol [Line: 2](contracts/libraries/NFTIdentifier.sol#L2)

	pragma solidity ^0.8.19;

Outdated compiler version:

Update to the most recent version of Solidity (0.8.24), by changing the pragma as follows:

pragma solidity 0.8.24;

Floating pragma:

Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of pragma solidity ^0.8.0;, use pragma solidity 0.8.24;

Remediation Plan

ACKNOWLEDGED : The Haqq team acknowledged this issue.

The NFTRegistry contract incorporates both the Ownable pattern and Role-Based Access Control (RBAC) pattern for managing access and permissions. While both systems are robust for access control in their respective contexts, employing them concurrently within the same contract can introduce complexity and confusion.

contract NFTRegistry is INFTRegistry, OwnableUpgradeable, AccessControlUpgradeable {

This overlap may lead to inconsistencies in access control logic, making the contract harder to maintain and potentially increasing the risk of security vulnerabilities. Specifically, the duplicate mechanism could cause confusion regarding which system takes precedence in various situations, complicate the assignment and revocation of roles and ownership, and make it more challenging to audit and reason about the contract's security model.

It is important to mention that, in regular scenarios, the DEFAULT_ADMIN_ROLE is already granted to the deployer address, when dealing with role-based access control default implementation by OpenZeppelin.

Evaluate the requirements regarding access control. If a more granular control is required, then it is recommended to use Role-based access control (RBAC).

On the other hand, if there is no need for granular control over authorizations and there is no need for having different types of roles, then it is advisable to use Ownable2Step.

Remediation Plan

SOLVED : The Haqq team solved this issue in the commit id 1d6c5a0913e01ac2400d574822b9da169a79d50b.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in Official Solidity Documentation.

Remediation Plan

ACKNOWLEDGED : The Haqq team acknowledged this issue.

In the ERC721H and ERC1155H contracts, the _setRoyalties function does not validate if the royaltyPercent parameter is below 100. This oversight could potentially allow for setting a royalty percentage that exceeds 100%, which is illogical and could lead to unexpected behavior or exploitation.

    function _setRoyalties(uint256 tokenId, address signer, uint256 royaltyPercent) internal {
        royalties[tokenId] = Royalties({nftCreator: signer, royaltyPercent: royaltyPercent});
    }

In the Listing Contract, the setFixedComPercent function similarly lacks validation for its newFixedComPercent parameter, failing to ensure it is below 100.

- contracts/Listing.sol [Line: 137-139](contracts/Listing.sol#L137-139)

    function setFixedComPercent(uint256 newFixedComPercent) external onlyOwner {
        fixedComPercent = newFixedComPercent;
    }

The same happens within the setAuctionComPercent in Auction contract:

- contracts/Auction.sol [Line: 68-70](contracts/Auction.sol#L68-70)

    function setAuctionComPercent(uint256 newAuctionComPercent) external onlyOwner {
        auctionComPercent = newAuctionComPercent;
    }

This poses a risk of setting a commission percentage that exceeds 100%, leading to impractical or exploitative scenarios.

Additionally, the contracts lack checks for the address(0) (zero address) when assigning values to address state variables. This is found in the ERC1155H and ERC721H contracts. Assigning the zero address to critical state variables can lead to loss of control over these variables or potentially lock assets within the contract without a way to interact with them.

- contracts/tokens/ERC1155H.sol [Line: 41](contracts/tokens/ERC1155H.sol#L41)

	        systemAddress = _systemAddress;

- contracts/tokens/ERC1155H.sol [Line: 87](contracts/tokens/ERC1155H.sol#L87)

	        systemAddress = target;

- contracts/tokens/ERC721H.sol [Line: 43](contracts/tokens/ERC721H.sol#L43)

	        systemAddress = _systemAddress;

- contracts/tokens/ERC721H.sol [Line: 87](contracts/tokens/ERC721H.sol#L87)

	        systemAddress = target;

Royalty percentage validation:

It is crucial to add input validation checks in the _setRoyalties, setFixedComPercent and setAuctionComPercent functions to ensure the percentages passed as parameters do not exceed 100. This can be done by using the following syntax.

Error declaration:

error WrongPercentage(uint256 _percentage);

1. In ERC721H and ERC1155H contracts - _setRoyalties function:

if (royaltyPercent > 100) revert WrongPercentage(royaltyPercent);

2. In Listing contract - setFixedComPercent function:

if (newFixedComPercent > 100) revert WrongPercentage(newFixedComPercent);

3. In Auction contract - setAuctionComPercent function:

if (newAuctionComPercent > 100) revert WrongPercentage(newAuctionComPercent);

These checks will prevent setting illogical values for royalties and commissions, safeguarding against unintended behavior or misuse.

Validation against the Zero Address:

Before assigning values to address state variables, it is essential to validate that the incoming address is not the zero address. This can prevent scenarios where the contract's functionality is compromised due to the unintentional assignment of critical variables to an unusable address.

Error declaration:

error ZeroAddressNotAllowed();

Implement a check using the following syntax:

if (_systemAddress == address(0)) revert ZeroAddressNotAllowed();

This validation should be added before every assignment operation to address state variables across all the contracts where such assignments occur, particularly in the lines highlighted within the ERC1155H and ERC721H contracts.

Remediation Plan

SOLVED : The Haqq team solved this issue in the commit id 1d6c5a0913e01ac2400d574822b9da169a79d50b.

During the analysis of the contracts under scope, specifically in Auction and Listing contracts, it was identified the use of magic numbers. This can reduce code readability, as some literals might not be so obvious or intuitive.

- contracts/Auction.sol [Line: 147](contracts/Auction.sol#L147)

	            (nftCreator, royalityPercent) = IERC2981(nftAddress).royaltyInfo(nftID, 100);

- contracts/Listing.sol [Line: 125](contracts/Listing.sol#L125)

	        _saleListingID = 1;

- contracts/Listing.sol [Line: 297](contracts/Listing.sol#L297)

	            list(List.FIXEDSALE, true, nftAddress, nftID, msg.sender, price, 0, expiration, 1);

- contracts/Listing.sol [Line: 299](contracts/Listing.sol#L299)

	            emit ListedFixedSale(nftAddress, nftID, msg.sender, price, expiration, 1, saleListingID);

- contracts/Listing.sol [Line: 388](contracts/Listing.sol#L388)

	            (nftCreator, royalityPercent) = IERC2981(nftAddress).royaltyInfo(nftID, 100);

- contracts/Listing.sol [Line: 476](contracts/Listing.sol#L476)

	            list(List.AUCTIONSALE, true, nftAddress, nftID, msg.sender, minPrice, startTime, endTime, 1);

- contracts/Listing.sol [Line: 478](contracts/Listing.sol#L478)

	            emit ListedAuctionSale(nftAddress, nftID, msg.sender, minPrice, startTime, endTime, 1, saleListingID);

It is recommended to declare constants and use it across the code, instead of literals or "magic numbers", for improved code readability and maintenance.

For example:

uint16 someConstant = 256;

Remediation Plan

ACKNOWLEDGED : The Haqq team acknowledged this issue.

The nonReentrant modifier should be placed before all other modifiers. It is a best practice to prevent reentrancy occurrences in other modifiers.

- contracts/Listing.sol [Line: 345](contracts/Listing.sol#L345)

	    function buyFixedSale(uint256 saleListingID) external payable existingFixedSaleListing(saleListingID) nonReentrant {

It is best practice to place the nonReentrant modifier after all other modifiers. In the specific occurrence, in Listing contract, buyFixedSale function:

	    function buyFixedSale(uint256 saleListingID) external payable nonReentrant existingFixedSaleListing(saleListingID) {

Remediation Plan

SOLVED : The Haqq team solved this issue in the commit id 1d6c5a0913e01ac2400d574822b9da169a79d50b.

Declaring functions as public when they are only meant to be accessed externally results in higher gas costs compared to marking them as external.

This is because public functions generate additional bytecode to handle both internal and external calls, whereas external functions are optimized for calls made from outside the contract. Switching to external to functions that do not require internal access can optimize contract efficiency by reducing gas consumption during execution.

- contracts/Auction.sol [Line: 163](contracts/Auction.sol#L163)

	    function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) {

- contracts/Auction.sol [Line: 167](contracts/Auction.sol#L167)

	    function onERC1155Received(address, address, uint256, uint256, bytes memory) public virtual returns (bytes4) {

- contracts/Auction.sol [Line: 171](contracts/Auction.sol#L171)

	    function onERC1155BatchReceived(

- contracts/NFTRegistry.sol [Line: 78](contracts/NFTRegistry.sol#L78)

	    function isWhitelisted(address nftAddress, uint256 nftID) public view override returns (bool whitelisted) {

- contracts/helpers/Registry.sol [Line: 82](contracts/helpers/Registry.sol#L82)

	    function getContract(string memory name) public view returns (address) {

- contracts/tokens/ERC1155H.sol [Line: 101](contracts/tokens/ERC1155H.sol#L101)

	    function burn(address account, uint256 tokenId, uint256 value) public {

- contracts/tokens/ERC1155H.sol [Line: 114](contracts/tokens/ERC1155H.sol#L114)

	    function burnBatch(address account, uint256[] memory tokenIds, uint256[] memory values) public {

- contracts/tokens/ERC721H.sol [Line: 108](contracts/tokens/ERC721H.sol#L108)

	    function burn(uint256 tokenId) public {

Functions that are not meant to be accessed internally should be marked as external.

Remediation Plan

SOLVED : The Haqq team solved this issue in the commit id 1d6c5a0913e01ac2400d574822b9da169a79d50b.