Blur Exchange contest - enckrish's results

Lines of code

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

Vulnerability details

Impact

BlurExchange.execute fetches the amount to transfer from the matching policies. StandardPolicyERC1155 doesn't take into account Order.amount and always returns 1 for the amount. Due to this only 1 token is transferred. This results in buyer paying for N tokens but getting only 1.

Only (suboptimal) way to get by with this in the current setting will be:

1. Seller sets Order.price to be per token.
2. Both parties increment their nonces and supply new signatures repeatedly after each execute call, if they want to buy more tokens.

Proof of Concept

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

Tools Used

Manual inspection

Recommended Mitigation Steps

Return Order.amount in StandardPolicyERC1155.sol instead of 1 for amount.

Low Severity Issues

[L-1] Missing zero address checks

BlurExchange.initialize doesn't check for zero address for _weth, _executionDelegate, _policyManager, and _oracle.

[L-2] ERC-20 transferFrom value not checked

transferFrom return value, which is then returned by ExecutionDelegate.transferERC20 is not checked to be true in BlurExchange._transferTo. Some tokens like ZRX, return false instead of reverting. Since the contracts are likely to use only WETH, this should not affect the proper functioning.

[L-3] ExecutionDelegate.transferERC20 may fail for non-standard tokens

ExecutionDelegate.transferERC20 assumes that transferFrom will always return a bool value. Tokens like USDT don't return a bool, so transferERC20 will be reverted. Consider using a low-level call for transferFrom and returning the success status if it is necessary to return a bool. It will not be an issue if only WETH is used, which follows the standard.

[L-4] Transfer of zero tokens

Few tokens choose to revert when trying to transfer zero tokens. When using those tokens, the transaction may get reverted in _transferFees if any recipient gets zero tokens. WETH doesn't revert on zero token transfer. To be on the safe side, consider checking if amount > 0 before transfers.

[L-5] Centralization of privileges

The contracts use ownerOnly modifier for many crucial setter functions and can break the protocol if someone gets the access to the owner wallet. Consider using a multi-sig wallet or making a DAO the owner to decrease the risks.

[L-6] Use type(uint256).max for Order.expirationTime for oracle cancellations

Currently orders with expirationTime set to 0 are settleable at any time subject to fulfillment of other conditions. Since by default it is set to 0 only, missing initialization will cause issues. It is suggested to set it to type(uint256).max explicitly, for oracle cancellations to prevent this.

Non-Critical

[Q-1] pragma abicoder v2 may be omitted

Abicoder v2 is enabled by default in Solidity 0.8.0 and above.

[Q-2] Use block.chainid to access the chain ID

In BlurExchange.initialize, it is recommended to use block.chainid instead of passing it as a parameter. It reduces risk of manual error and saves about 700 gas.

[Q-3] Missing revert reason

Revert reason is not specified in BlurExchange.execute https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L134

[Q-4] BlurExchange.cancelOrder should revert if fails

A user cannot easily know if BlurExchange.cancelOrder failed without noticing a missing log. It is recommended to revert with a suitable message on failing. To ensure proper functioning of cancelOrders , the core logic can be extracted to an internal function that returns bool for success status, which can be used to revert in cancelOrder if it returns false, or can be ignored in cancelOrders.

[Q-5] Comparison with boolean constants

There are 5 instances in ExecutionDelegate.sol and BlurExchange.sol where there is an unnecessary comparison with boolean constants. Boolean variables need not be explicitly compared using a == operator. An example of this is: https://github.com/code-423n4/2022-10-blur/blob/main/contracts/ExecutionDelegate.sol#L77

[Q-6] Unused variable merklePath in BlurExchange._validateOracleAuthorization

Unused variable at: https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L388. Also saves 400 gas at deployment.

[Q-7] Use appropriate names for variables

ExecutionDelegate.contracts should be renamed to something like contractsApproved to better reflect the use.

[Q-8] Unnecessary use of payable

OrderStructs.Fee.recipient is declared of type address payable even though it is explicitly converted to payable in BlurExchange._transferTo. payable should be removed to increase consistency across other address members in other structs.

[Q-9] Use checks-effects-interaction pattern

While there is no risk due to the usage of a reentrancy-guard, BlurExchange.execute updates cancelledOrFilled after potentially dangerous external calls, which is not recommended practice. Consider moving these state changes above the _execute*transfer calls. https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L164-165

[G-1] Cache the result of sell.order.listingTime <= buy.order.listingTime

Caching this result in BlurExchange.execute (https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L168) reduces the deploy gas usage by 6300. An example implementation would be:

bool sellFirst = sell.order.listingTime <= buy.order.listingTime;
emit OrdersMatched(
    sellFirst ? sell.order.trader : buy.order.trader,
    !sellFirst ? sell.order.trader : buy.order.trader,
    ...
);

[G-2] Redundant EnumerableSet.contains call before adding/removing policies

EnumerableSet.add and EnumerableSet.remove, both internally call contains and return false if the operation failed based on contain's return value. Removal of the two redundant checks in PolicyManager will result in decrease of 8200 gas at deployment of BlurExchange and about 260 when calling the involved external/public functions. Example implementation:

function addPolicy(address policy) external override onlyOwner {
    require(_whitelistedPolicies.add(policy), "Already whitelisted");
    ...

function removePolicy(address policy) external override onlyOwner {
    require(_whitelistedPolicies.remove(policy), "Not whitelisted");
    ...

[G-3] Use uint256 for reentrancyLock

Using a bool for reentrancyLock increases the gas costs, as compared to uint256. Changing the type will result in a decrease of 4000 gas at deployment of BlurExchange and 250 gas for every execute call. In general, it is not recommended to use smaller variables since they use more gas. More details can be found here: https://ethereum.stackexchange.com/questions/3067/why-does-uint8-cost-more-gas-than-uint256.

Example implementation:

uint reentrancyLock = 0;

/* Prevent a contract function from being reentrant-called. */
modifier reentrancyGuard {
    require(reentrancyLock == 0, "Reentrancy detected");
    reentrancyLock = 1;
    _;
    reentrancyLock = 0;
}

[G-4] Loop variable increment can be done inside unchecked block

There are 5 instances across the codebase, where the loop is bounded to the length of an array. In these cases, the loop variable can not overflow, and can be safely incremented inside a unchecked block to save gas.