Blur Exchange contest - saian's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

Vulnerability details

Impact

When filling ERC1155 token orders with token amount greater than 1, _canMathOrders returns amount as 1 instead of returning the actual amount of tokens order.amount. So the buyers will pay order.price payment tokens for the entire amount of tokens, but gets only 1 token in return

Proof of Concept

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33

    return (
        (makerAsk.side != takerBid.side) &&
        (makerAsk.paymentToken == takerBid.paymentToken) &&
        (makerAsk.collection == takerBid.collection) &&
        (makerAsk.tokenId == takerBid.tokenId) &&
        (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
        (makerAsk.price == takerBid.price),
        makerAsk.price,
        makerAsk.tokenId,
        1,
        AssetType.ERC1155
    );

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

    return (
        (makerBid.side != takerAsk.side) &&
        (makerBid.paymentToken == takerAsk.paymentToken) &&
        (makerBid.collection == takerAsk.collection) &&
        (makerBid.tokenId == takerAsk.tokenId) &&
        (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
        (makerBid.price == takerAsk.price),
        makerBid.price,
        makerBid.tokenId,
        1,
        AssetType.ERC1155
    );

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145

    (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) = _canMatchOrders(sell.order, buy.order);

Tools Used

Manual analysis

Recommended Mitigation Steps

In matching policy use actual makerBid.amount and makerAsk.amount in the return statement

Low severity findings

Malicious owner can steal assets

Users approve ExecutionDelegate contract for transferring assets when order is filled. A malicious owner can approve arbitrary contract/EOA to transfer assets out of the contract. Consider removing the function and storing the exchange address in the constructor, or use a multi-sig contract as the owner address

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L36

    function approveContract(address _contract) onlyOwner external { 
        contracts[_contract] = true;
        emit ApproveContract(_contract);
    }

approveContract can approve EOAs

approveContract is used to approve contracts to transfer assets out of the contract. But since no isContract exists, even EOAs can be approved to transfer assets

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L36

    function approveContract(address _contract) onlyOwner external { 
        contracts[_contract] = true;
        emit ApproveContract(_contract);
    }

Add isContract function to check if the address is a contract

    function approveContract(address _contract) onlyOwner external { 
        require(isContract(_contract), "...");
        contracts[_contract] = true;
        emit ApproveContract(_contract);
    }

_blockrange is not validated

In setBlockRange the _blockRange value is not validated, so it can be set to 0, preventing oracle authorisation, or set to a very high value, allowing old signatures/ Add input validation to validate the blockrange value

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L246

    function setBlockRange(uint256 _blockRange)
        external
        onlyOwner
    {
        blockRange = _blockRange;
        emit NewBlockRange(blockRange); 
    }

If payment token is WETH, msg.value should be 0

Since execute can receive both ether and WETH, checks should be added to prevent eth transfer when the payment token is WETH to prevent ether from being locked in the contract Add function to retrived ether or accidentally sent ERC20 tokens from the contract

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L451

        if (paymentToken == address(0)) {
            require(msg.value == price);    
        }

checks should be added to prevent ether transfer if payment token is WETH Add function to retreive ether or accidentally sent ERC20 tokens

        if (paymentToken == address(0)) {
            require(msg.value == price,"...");    
        } else {
            require(msg.value == 0,"...")
        }

Use call() instead of transfer()

Since transfer forwards a fixed amount of gas 2300, the calls may fail if gas costs change in the future, or if the receiver is a smart contract that consumes more than 2300 gas. Use .call() to send ether instead of transfer.

refer-https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L508

    payable(to).transfer(amount);

Non-critical findings

Missing comments

missing @amount param in

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L523

missing @returns param in

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/MerkleVerifier.sol#L31

    /**
     * @dev Compute the merkle root
     * @param leaf leaf
     * @param proof proof
     */
    function _computeRoot(

missing @returns in

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L117

    /**
     * @dev Transfer ERC20 token
     * @param token address of the token
     * @param from address of the sender
     * @param to address of the recipient   
     * @param amount amount
     */
    function transferERC20(address token, address from, address to, uint256 amount)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/PolicyManager.sol#L45

    /**
     * @notice Returns if a policy has been added
     * @param policy address of the policy to check 
     */
    function isPolicyWhitelisted(address policy) external view override returns (bool)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/PolicyManager.sol#L52

    /**
     * @notice View number of whitelisted policies   
     */
    function viewCountWhitelistedPolicies() external view override returns (uint256)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/PolicyManager.sol#L61

    /**
     * @notice See whitelisted policies 
     * @param cursor cursor
     * @param size size
     */
    function viewWhitelistedPolicies(uint256 cursor, uint256 size)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L546

    /**
     * @dev Determine if the given address exists
     * @param what address to check
     */
    function _exists(address what) 

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L467

    /**
     * @dev Charge a fee in ETH or WETH
     * @param fees fees to distribute
     * @param paymentToken address of token to pay in
     * @param from address to charge fees
     * @param price price of token
     */
    function _transferFees(
        Fee[] calldata fees,
        address paymentToken,
        address from,
        uint256 price
    ) internal returns (uint256)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L414

    /**
     * @dev Call the matching policy to check orders can be matched and get execution parameters
     * @param sell sell order
     * @param buy buy order
     */
    function _canMatchOrders(Order calldata sell, Order calldata buy)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L405

    /**
     * @dev Wrapped ecrecover with safety check for v parameter
     * @param v v
     * @param r r
     * @param s s
     */
    function _recover(
        bytes32 digest,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal pure returns (address)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L373

    /**
     * @dev Verify the validity of oracle signature
     * @param orderHash hash of the order
     * @param signatureVersion signature version
     * @param extraSignature packed oracle signature
     * @param blockNumber block number used in oracle signature
     */
    function _validateOracleAuthorization

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L342

    /**
     * @dev Verify the validity of the user signature
     * @param orderHash hash of the order
     * @param trader order trader who should be the signer
     * @param v v
     * @param r r
     * @param s s
     * @param signatureVersion signature version
     * @param extraSignature packed merkle path
     */
    function _validateUserAuthorization(
        bytes32 orderHash,
        address trader,
        uint8 v,
        bytes32 r,
        bytes32 s,
        SignatureVersion signatureVersion,
        bytes calldata extraSignature
    ) internal view returns (bool)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L289

    /**
     * @dev Verify the validity of the signatures
     * @param order order
     * @param orderHash hash of order
     */
    function _validateSignatures(Input calldata order, bytes32 orderHash)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L256

    /**
     * @dev Check if the order can be settled at the current timestamp
     * @param listingTime order listing time
     * @param expirationTime order expiration time
     */
    function _canSettleOrder(uint256 listingTime, uint256 expirationTime)

Unused import

Files imported are not used

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L5

    import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L8

    import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

Events not indexed

Events should use indexed fields, indexed fields help off-chain scripts to efficiently filter these events

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L85-L91

    event OrderCancelled(bytes32 hash);
    event NonceIncremented(address trader, uint256 newNonce);   

    event NewExecutionDelegate(IExecutionDelegate executionDelegate);
    event NewPolicyManager(IPolicyManager policyManager);
    event NewOracle(address oracle);
    event NewBlockRange(uint256 blockRange);

Missing revert messages

Require statements does not have revert messages, So if the require condition fails, the transaction will revert without providing informative messages to the user.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L134

    require(sell.order.side == Side.Sell); 

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L452

    require(msg.value == price); 

abicoder v2 is used by default from 0.8.0

abicoder is used by default from solidity version 0.8.0

https://blog.soliditylang.org/2020/12/16/solidity-v0.8.0-release-announcement/

Switching from non-zero to non-zero is more efficient

By switching from non-zero to non-zero value, expensive SSTORE from 0/false to non-zero can be avoided. and when boolean is used, writing to storage costs more than uint256 as each write operation has to perform extra read operation to correctly place the boolean value in the slot. So switching from 0/false to 1/true can be replaced by 1 and 2

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/86bd4d73896afcb35a205456e361436701823c7a/contracts/security/ReentrancyGuard.sol#L29-L33

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/ReentrancyGuarded.sol#L13

    modifier reentrancyGuard {
        require(!reentrancyLock, "Reentrancy detected");
        reentrancyLock = true;
        _;
        reentrancyLock = false;
    }

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L43

    function open() external onlyOwner {    
        isOpen = 1;
        emit Opened();
    }
    function close() external onlyOwner {
        isOpen = 0;
        emit Closed();
    }

Reduce size of revert string

Revert strings that are longer than 32 bytes requires at least one additional mstore, along with additional overhead for computing memory offset, etc. Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L22

    require(contracts[msg.sender], "Contract is not approved to make transfers");

Comparison with bool literal

Comparison to boolean literal is more expensive than directly checking the value. Comparison with false can be replaced by using the ! operator with the value

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L77

    require(revokedApproval[from] == false, "User has revoked approval");

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L92

    require(revokedApproval[from] == false, "User has revoked approval");

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L108

    require(revokedApproval[from] == false, "User has revoked approval");   

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L124

    require(revokedApproval[from] == false, "User has revoked approval");   

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L267

    (cancelledOrFilled[orderHash] == false)

constants can be internal/private

Marking constants as private/internal saves gas on deployment as compiler does not have to create a getter function for these variables, these variables can still be read from the verfied source code or the bytecode.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L20-L33

    bytes32 constant public FEE_TYPEHASH = keccak256(
        "Fee(uint16 rate,address recipient)"
    );
    bytes32 constant public ORDER_TYPEHASH = keccak256(
        "Order(address trader,uint8 side,address matchingPolicy,address collection,uint256 tokenId,uint256 amount,address paymentToken,uint256 price,uint256 listingTime,uint256 expirationTime,Fee[] fees,uint256 salt,bytes extraParams,uint256 nonce)Fee(uint16 rate,address recipient)"
    );
    bytes32 constant public ORACLE_ORDER_TYPEHASH = keccak256(
        "OracleOrder(Order order,uint256 blockNumber)Fee(uint16 rate,address recipient)Order(address trader,uint8 side,address matchingPolicy,address collection,uint256 tokenId,uint256 amount,address paymentToken,uint256 price,uint256 listingTime,uint256 expirationTime,Fee[] fees,uint256 salt,bytes extraParams,uint256 nonce)"
    );
    bytes32 constant public ROOT_TYPEHASH = keccak256(
        "Root(bytes32 root)"
    );

    bytes32 constant EIP712DOMAIN_TYPEHASH = keccak256(
        "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
    );

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L57

    string public constant name = "Blur Exchange";  
    string public constant version = "1.0";

Cache array length

Array length can be cached in a variable and used in the loop instead of read from storage in every iteration

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L77

    for (uint256 i = 0; i < fees.length; i++)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/MerkleVerifier.sol#L38

    for (uint256 i = 0; i < proof.length; i++)

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L199

    for (uint8 i = 0; i < orders.length; i++

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L476

    for (uint8 i = 0; i < fees.length; i++) 

Function used once can be inlined

Functions used once can be inlined to save gas on performing JUMP between the functions.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L548

    function _exists(address what)
        internal
        view
        returns (bool)
    {
        uint size;
        assembly {
            size := extcodesize(what)
        }
        return size > 0;
    }

bytes32 instead of string

If data can fit into 32 bytes, then using bytes32 instead of string is cheaper, as string is a dynamically sized data structures and therefore consumes more gas.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L57

    string public constant name = "Blur Exchange";  
    string public constant version = "1.0";

Custom errors

Custom error from solidity 0.8.4 are cheaper than revert strings with cheaper deployment cost and runtime cost when the revert condition is met.

https://blog.soliditylang.org/2021/04/21/custom-errors/

Use unchecked to save gas

If an expression cannnot overflow/underflow it can be placed in a unchecked block to avoid the implicit overflow/underflow checks introduced in 0.8.0 and save gas

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L485

    require(totalFee <= price, "Total amount of fees are more than the price");

    /* Amount that will be received by seller. */
    uint256 receiveAmount = price - totalFee; 

Variables less than 32 bytes

Due to how the EVM natively works on 256 bit numbers, using a 8 bit number in for-loops introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L199

    for (uint8 i = 0; i < orders.length; i++)

Avoid initialization with default values

When variables are not set, it is assumed to have it's default value(0 for uint, false for bool, address(0) for address). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/ReentrancyGuarded.sol#L10

    bool reentrancyLock = false;

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L475

        uint256 totalFee = 0; 

Use ++x instead of x += 1

Pre-increment ++x costs less when compared to x += 1 or x++ for unsigned integers

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L208

    nonces[msg.sender] += 1;

Use function arguments instead of storage variables in event emits

In event emits using local variables or function arguments instead of storage variable can avoid storage read and save gas

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L221

        emit NewExecutionDelegate(executionDelegate);   

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L230

        emit NewPolicyManager(policyManager); 

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L239

        emit NewOracle(oracle); 

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L247

        emit NewBlockRange(blockRange); 

Unnecessary variables

Variables _v, _r and _s are unnecessary and the values can be directly assigned to the variabesl v, r and s

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L388

    (bytes32[] memory merklePath, uint8 _v, bytes32 _r, bytes32 _s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));
    v = _v; r = _r; s = _s; 

can be replaced with

    (bytes32[] memory merklePath, v, r, s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));