Blur Exchange contest - d3e4'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

An order can be placed for an arbitrary amount, which is relevant for ERC1155. But when matched and executed only 1 token is transferred. This can lead to problems with accounting for the user, expecting a transfer of Order.amount tokens, potentially with a loss of funds as a consequence.

Proof of Concept

StandardPolicyERC1155.sol hardcodes a return value of 1, which is passed to BlurExchange as the amount used in the transferERC1155() function.

Tools Used

Code inspection

Recommended Mitigation Steps

Consider amending StandardPolicyERC1155.sol to make use of the Order.amount for ERC1155, either by allowing for the transfer of more than one token or by returning false in canMatchMakerAsk() and canMatchMakerBid for a match where Order.amount != 1 (e.g. by adding ... && makerAsk.amount == 1 && takerBid.amount == 1 to the bool return).

Lines of code

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

Vulnerability details

Impact

Owner can steal users' funds and tokens.

Proof of Concept

The owner of ExecutionDelegate can approve any contract to call it, e.g. a malicious contract which can then drain all funds and tokens approved by users to ExecutionDelegate. Users should only trust ExecutionDelegate if they can trust who can call it. There are two ways to ensure this. Either let approved callers be immutable, or have the user (instead of the owner) approve the contract to call transfers on his behalf.

Tools Used

Code inspection

Recommended Mitigation Steps

Let approved callers (contracts) be immutable, or remove owner privilege to approve callers and disallow all callers by default and let each owner grant approval to specific calling contracts.

Ownership issues

BlurExchange, ExecutionDelegate and PolicyManager inherits from OpenZeppelin's Ownable or OwnableUpgradeable, which implement renounceOwnership and transferOwnership. A, perhaps accidental, misuse of these may leave the contracts without an owner, thus rendering many critical functions modified by onlyOwner inaccessible. Consider reimplementing renounceOwnership to disable it, and reimplementing transferOwnership as a two-step process where the new owner is nominated for and has to accept the ownership.

 

weth issues _weth = weth is missing an address(0) check and weth has no function to change it after initalization (which it should NOT have!), yet it is declared as a public variable. If it is incorrectly set the exchange will be useless, as sell orders become impossible. Consider making it a (private) constant.

 

Signature malleability _recover() is susceptible to signature malleability, which allows replay attacks. Since the order hash of executed orders is stored this is currently not an issue, but if the contract logic is changed this vulnerabilty might materialize. Consider checking that s is in the lower half range, i.e. s <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0, or using OpenZeppelin's ECDSA.sol.

 

No check for invalid ecrecover() return value ecrecover() returns 0 on invalid input. Consider reverting _recover() if ecrecover() returns 0.

 

StandardPolicyERC1155 doesn't allow for batch transfers A main feature of ERC1155, compared to ERC721, is batch transfers of tokens. But the amount to transfer is hardcoded as 1 in StandardPolicyERC1155.sol at L33 and L59. Consider amending the policy to make use of the Order.amount for ERC1155.

 

Missing address(0) check Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L97 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L100

 

Explicitly mark visibility of state Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/ReentrancyGuarded.sol#L10 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L37

 

Ambiguous parameter name order The parameter (Input calldata order](https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L291) is a struct Input which in itself has an Order also named order, leading to ambiguity. Consider renaming the outer Input calldata order e.g. input.

 

Unused name of return variable Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L115 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L127 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L142 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/ERC1967Proxy.sol#L29

 

Redundant/unused imports At BlurExchange.sol#L5 Initializable.sol is imported, but not explicitly inherited in the contract. It is however imported and inherited in the two following imports of UUPSUpgradeable.sol and OwnableUpgradeable.sol.

ERC20.sol is imported at BlurExchange.sol#L8 but never used.

 

Missing NatSpec Most of the code in scope is missing NatSpec.

 

Typos "musted" -> "must"

 

Incorrect comments The function _exists(address what) does not "Determine if the given address exists" but whether what is a deployed contract.

[G-01] Code repetition in StandardPolicyERC721.sol and StandardPolicyERC1155.sol [G-02] Redundant check [G-03] ReentrancyGuarded.sol has a cheaper implementation [G-04] Comparing to constant bool [G-05] Declaring constants as private rather than public saves deployment gas [G-06] Fixed bytes is cheaper than string [G-07] Function not called by the contract itself can be external instead of public [G-08] Use calldata instead of memory for function parameters that are read-only [G-09] Unnecessary two-step assignment [G-10] Emit a memory, rather than storage, variable when available [G-11] Pre-compute .length before repeated usage, especially in loops [G-12] Cache/precompute _whitelistedPolicies.length() - cursor [G-13] Pre-incrementing is cheaper than post-incrementing [G-14] Uncheck integer calculations [G-15] require is cheaper than if [G-16] Custom errors are cheaper than require-statements

 

[G-01] Code repetition in StandardPolicyERC721.sol and StandardPolicyERC1155.sol

StandardPolicyERC721.sol and StandardPolicyERC1155.sol are identical up to token names ("ERC721" and "ERC1155"). Consider combining them into one contract. Also, within these contracts, the functions canMatchMakerAskand canMatchMakerBid are identical up to the order of Bid/Ask in the variables makerAsk/takerBid. Consider combining these two functions into one, possibly into a private helper function if you want to retain the two function names canMatchMakerAskand canMatchMakerBid. I understand that they were written as two near copies in order to match IMatchingPolicy, but as BlurExchange deals specifically and only with ERC721 and ERC1155 I stand by this consideration to combine them.

 

[G-02] Redundant check

The check in PolicyManager.sol#L26 is not necessary. add only returns false if _whitelistedPolicies.contains(policy). If the error message is needed it can be triggered directly by the bool returned from add. Similarly in PolicyManager.sol#L37

 

[G-03] ReentrancyGuarded.sol has a cheaper implementation

reentrancyLock can be private. It is also cheaper to implement it as a uint256 rather than a bool. For reference see OpenZeppelin's implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol.

 

[G-04] Comparing to constant bool

Replace e.g. foo == false with !foo.

Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L267 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L92 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L108 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L124

 

[G-05] Declaring constants as private rather than public saves deployment gas

The compiler will give public constants a getter method, which costs gas during deployment. As they are constants they can be read directly from the verified source code.

Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L57 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L58 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L59 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L20 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L23 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L26 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L29 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L33

 

[G-06] Fixed bytes is cheaper than string

As a rule of thumb, use bytes for arbitrary-length raw byte data and string for arbitrary-length string (UTF-8) data. If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

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

 

[G-07] Function not called by the contract itself can be external instead of public

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

 

[G-08] Use calldata instead of memory for function parameters that are read-only

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

 

[G-09] Unnecessary two-step assignment

At BlurExchange.sol#L389 v, r and s are set via temporary return values from abi.decode(). Save three assignments (9 gas) by replacing L388-L389 with this:

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

 

[G-10] Emit a memory, rather than storage, variable when available

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

 

[G-11] Pre-compute .length before repeated usage, especially in loops

Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L199 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L476 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L75-L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/MerkleVerifier.sol#L38

 

[G-12] Cache/precompute _whitelistedPolicies.length() - cursor

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

 

[G-13] Pre-incrementing is cheaper than post-incrementing

Consider replacing e.g. i++ with ++i.

Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L199 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L208 (also consider putting it directly in the emit) https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L476 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/PolicyManager.sol#L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/MerkleVerifier.sol#L38

 

[G-14] Uncheck integer calculations

Where integers cannot over-/underflow they may be unchecked{} to save gas. In loops, make the unchecked increment in the end of the loop.

Instances: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L199 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L208 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L318 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L476 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L485 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/PolicyManager.sol#L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/EIP712.sol#L77 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/MerkleVerifier.sol#L38

 

[G-15] require is cheaper than if

In cancelOrder an if-clause is used to check whether the order is already cancelled or filled. If it is nothing happens, and if it isn't a state change occurs and an event is emitted. It is cheaper to use a require, which also has the benefit of directly informing the user that the order didn't change. Consider replacing if (!cancelledOrFilled[hash]) with require(cancelledOrFilled[hash]).

 

[G-16] Custom errors are cheaper than require-statements

See this. Consider replacing require-statements with a string message with a custom error in an if-revert.

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