mintVoucher vulnerable to re-entrancy

[cducrest]

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

audit-wiiqare-1/contracts/WiiQareVoucherV1.sol

Lines 67 to 75 in 0243110

	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();
	}

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);
    }

[liviuvlad-innovatorspark]

issue addressed in the this pr