brainbot-com / audit-wiiqare-1 Goto Github PK

Summary

The function to transfer and alter a voucher should check that a voucher exists before attempting to transfer / alter it.

## Vulnerability Detail

The functions transferVoucher() and alterVoucher() do not revert when called with a voucher that was not minted beforehand:

    function transferVoucher(
        uint256 voucherID,
        string memory ownerID
    ) public whenNotPaused onlyOwner {
        vouchers[voucherID].ownerID = ownerID;
        emit transferVoucherEvent(voucherID, ownerID);
    }

    function alterVoucher(
        uint256 voucherID,
        SharedStructs.Voucher memory voucher
    ) public whenNotPaused onlyOwner {
        vouchers[voucherID] = voucher;
        emit alterVoucherEvent(voucherID, voucher);
    }

Impact

This could result in confusion to the caller of the contract if they want to mint a voucher and transfer it to someone but they mistakenly call transferVoucher() before calling mintVoucher() resulting in the voucher being owned by the owner provided in mintVoucher(). This could also occur due to a reorg of caller transactions if the owner contract does not enforce an ordering of the calls.

This can lead to unforeseen event handling in the backend / frontend when events are received out of expected order.

References

Recommendation

Revert when the function is called with a non-existing voucher, e.g. by checking that vouchers[voucherID].value > 0 or with ERC721's _exists(voucherID) function.

Summary

This issue references minor code comments or gas optimizations that do not represent a vulnerability or security risk.

Detail

Remove redundant Voucher struct

The Voucher struct is defined both in WiiQareVoucherV1 and SharedStructs. All throughout the code only SharedStructs.Voucher is used. Remove WiiQareVoucherV1.Voucher to avoid mistakes in the future that could arise with inconsistencies between WiiQareVoucherV1.Voucher and SharedStructs.Voucher.

Fix typo in mintVoucherEvent

Event names the variable nftVoucer instead of nftVoucher.

Remove unused / useless code

The constructor sets _voucherID = 0; which is redundant as uint256 variables start with their default values of 0.

Remove unused internal functions _decrementVoucherID() and _resetVoucherID().

Voucher struct may cost more gas than necessary

The struct uses uint256 and string that take at least a full word in storage. Strings will take more than a word if they are more than 31 bytes long, consider replacing with bytes32 or uint256 where appropriate.

By looking at the tests, I understand that status takes a limited number of values (claimed or unclaimed) and could be replaced with an enum (taking 8 bits of storage). This could be tightly packed into a slot if value is converted to uint248:

library SharedStructs {
    enum Status {
        claimed,
        unclaimed
    }
    struct Voucher {
        uint248 value;
        Status status;
        string currencySymbol;
        string ownerID;
        string providerID;
        string beneficiaryID;
    }
}

This will save 20k gas on minting any new voucher but slightly increase the cost of reading value when status is not read (see https://docs.soliditylang.org/en/latest/internals/layout_in_storage.html).

Summary

The function mintVoucher() does not satisfy the "checks, effects, interactions" pattern and as such is vulnerable to re-entrancy / read-only re-entrancy attacks from the caller.

Vulnerability Detail

The mintVoucher() function calls _safeMint(msg.sender, _voucherID) before incrementing the voucher ID:

    function mintVoucher(
        SharedStructs.Voucher memory voucher
    ) public onlyOwner whenNotPaused {
        require(voucher.value > 0, "Value of voucher must be greater than 0");
        vouchers[_voucherID] = voucher;
        _safeMint(msg.sender, _voucherID);
        emit mintVoucherEvent(_voucherID, voucher);
        _incrementVoucherID();
    }

_safeMint() will call onERC721Received() on the recipient of the token in case it is a contract:

    function _safeMint(address to, uint256 tokenId) internal virtual {
        _safeMint(to, tokenId, "");
    }

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert("ERC721: transfer to non ERC721Receiver implementer");
                } else {
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

This gives the caller of mintVoucher() time to execute arbitrary code in a state where the voucher has been created and the token minted but the voucher ID has not been increased yet.

Impact

In the current code iteration, only the owner of WiiQareVoucherV1() can call the mintVoucher() function. If the owner is trusted, the impact is low. However, if the code evolves into a more decentralized version this could bring unbound consequences.

References

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7de6fd4a2604764497990bcc0013f95763713190/contracts/token/ERC721/ERC721.sol#L304-L315

Recommendation

Move the _safeMint() call responsible for the external call at the end of the function:

    function mintVoucher(
        SharedStructs.Voucher memory voucher
    ) public onlyOwner whenNotPaused {
        require(voucher.value > 0, "Value of voucher must be greater than 0");
        vouchers[_voucherID] = voucher;
-       _safeMint(msg.sender, _voucherID);
        emit mintVoucherEvent(_voucherID, voucher);
        _incrementVoucherID();
+       _safeMint(msg.sender, _voucherID);
    }