NFT Invoice - Truffles

The functions addEligibleHolder, removeEligibleHolder and isEligibleHolder do not perform verification to ensure that no addresses provided as parameters are the zero addresses. Consequently, there is a risk of accidentally setting an eligible holder address to the zero address, leading to unintended behavior or potential vulnerabilities in the future.

Code Location

contracts/Truffles.sol

function addEligibleHolder(address _org) public onlyAuthorized {
    s_eligibleHolders[_org] = true;
}

/// @notice remove eligible holders for NFT
/// @dev remove eligible address to eligibleHolders maping : onlyAuthorized
/// @param _org address of already eligible holder
function removeEligibleHolder(address _org) public onlyAuthorized {
    s_eligibleHolders[_org] = false;
}

contracts/Truffles.sol

function isEligibleHolder(address _holder)
    public
    view
    returns (bool _isEligibleHolder)
{
    return (s_eligibleHolders[_holder]);
}

SOLVED: The \client team solved this finding in commit fc36014: the function addEligibleHolder was updated with a validation step to ensure that the provided address is not the zero address before making modifications to the headlineholders' eligibility. Similarly, the removeEligibleHolder function now includes a validation check to confirm that the given address corresponds to an existing holder.

The settlePrivateInvoice function allows authorized addresses to mark the corresponding NFT of a private invoice as settled when the full amount of the invoice is paid. However, it lacks a check to verify that the provided NFT is of the private invoice type. Consequently, both private and public invoices could be settled indifferently. If the provided NFT ID corresponds to a public invoice, it is added to the s_isPrivateInvoicePaid mapping, even though its type does not match, leading to data inconsistency.

In addition, private invoices do not store the amount for the NFT unlike public ones, so the function also does not have a proper check to verify that the full amount of the invoice is paid.

Code Location

contracts/Truffles.sol

function mintPrivateInvoice(
    address to,
    uint256 tokenId,
    string memory uri
) public onlyAuthorized {
    _safeMint(to, tokenId);
    _setTokenURI(tokenId, uri);
    s_nftType[tokenId] = NftType.PrivateInvoice;
}

contracts/Truffles.sol

function settlePrivateInvoice(uint256 _nftID) public onlyAuthorized {
    require(_exists(_nftID), "NFT not minted");
    s_isPrivateInvoicePaid[_nftID] = true;

}

SOLVED: The \client team solved this finding in commit fc36014: A validation step was introduced to ensure that the provided NFT ID is that of a private invoice before modifying the s_isPrivateInvoicePaid mapping.

In the Authorizable contract, the addAuthorized function allows the owner to add an address to the list of authorized addresses. It requires that the provided address is not the zero address, and it can only be executed by the contract owner because of the onlyOwner modifier.

The removeAuthorized function allows the owner to remove an address from the list of authorized addresses. It requires that the provided address is not the zero address and is different from the senders, which has to be the owner.

The checks performed in the latter function are redundant, since the addAuthorized function already prevents adding the zero address as an authorized address. Therefore, this check is unnecessary and result in extra gas overhead.

Code Location

contracts/Authorizable.sol

function addAuthorized(address _toAdd) onlyOwner public {
    require(_toAdd != address(0));
    authorized[_toAdd] = true;
}

contracts/Authorizable.sol

function removeAuthorized(address _toRemove) onlyOwner public {
    require(_toRemove != address(0));
    require(_toRemove != msg.sender);
    authorized[_toRemove] = false;
}

SOLVED: The \client team solved this finding in commit fc36014: the redundant check was removed from the removeAuthorized function.

It was identified that the Owner cannot pause the Truffles contract. In the case of a security incident, this means that the owner lacks the ability to halt the minting or settlement of Invoices, potentially leading to further complications.

Code Location

• contracts/Truffles.sol

SOLVED: The \client team solved this finding in commit 2e2a367: The contract now incorporates a pausing mechanism, which introduces two additional functions: pauseContract and unpauseContract. This enhancement is complemented by the inclusion of the whenNotPaused modifier within several functions, including mintPrivateInvoice, settlePrivateInvoice, mintPublicInvoice, and _beforeTokenTransfer.

The Truffles contract uses the Solidity pragma ^0.8.9. It's essential to deploy the contract with the exact compiler version and flags that have undergone thorough testing. Locking the pragma to a specific version helps to ensure that contracts are not accidentally deployed using outdated compiler versions, which might introduce bugs negatively impacting the contract system, or excessively new pragma versions that haven't undergone extensive testing.

Code Location

contracts/Truffles.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

SOLVED: The \client team solved this finding in commit fc36014: the pragma version has been locked and upgraded to match the version used in the Authorizable contract.

contracts/Truffles.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;