Blur Exchange contest - Rolezn's results

Summary<a name="Summary">

Low Risk Issues

Total: 19 contexts over 5 issues

Non-critical Issues

Total: 24 contexts over 5 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Missing Checks for Address(0x0)

Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

<ins>Proof Of Concept</ins>

115: function initialize: address _oracle

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L115

<ins>Recommended Mitigation Steps</ins>

Consider adding explicit zero-address validation on input parameters of address type.

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> Vulnerable To Cross-chain Replay Attacks Due To Static DOMAIN_SEPARATOR

The protocol calculates the chainid it should assign during its execution and permanently stores it in an immutable variable. Should Ethereum fork in the feature, the chainid will change however the one used by the permits will not enabling a user to use any new permits on both chains thus breaking the token on the forked chain permanently.

Please consult EIP1344 for more details: https://eips.ethereum.org/EIPS/eip-1344#rationale

<ins>Proof Of Concept</ins>

121: DOMAIN_SEPARATOR = _hashDomain(EIP712Domain({
            name              : NAME,
            version           : VERSION,
            chainId           : block.chainid,
            verifyingContract : address(this)
        }));

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L121

136: DOMAIN_SEPARATOR = _hashDomain(EIP712Domain({
            name              : NAME,
            version           : VERSION,
            chainId           : block.chainid,
            verifyingContract : address(this)
        }));

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L136

<ins>Recommended Mitigation Steps</ins>

The mitigation action that should be applied is the calculation of the chainid dynamically on each permit invocation. As a gas optimization, the deployment pre-calculated hash for the permits can be stored to an immutable variable and a validation can occur on the permit function that ensure the current chainid is equal to the one of the cached hash and if not, to re-calculate it on the spot.

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> Missing Contract-existence Checks Before Low-level Calls

Low-level calls return success if there is no code present at the specified address.

<ins>Proof Of Concept</ins>

631: (bool success,) = payable(to).call{value: amount}("");

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L631

47: (bool success,) = payable(msg.sender).call{value: amount}("");

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L47

<ins>Recommended Mitigation Steps</ins>

In addition to the zero-address checks, add a check to verify that <address>.code.length > 0

<a href="#Summary">[LOW‑4]</a><a name="LOW&#x2011;4"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>

4: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
5: import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
6: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L4-L6

4: import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
5: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L4-L5

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

<a href="#Summary">[LOW‑5]</a><a name="LOW&#x2011;5"> Upgrade OpenZeppelin Contract Dependency

An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

<ins>Proof Of Concept</ins>

@openzeppelin/contracts: 4.4.1

https://github.com/code-423n4/2022-11-non-fungible/tree/main/package.json#L61

@openzeppelin/contracts-upgradeable: ^4.6.0

https://github.com/code-423n4/2022-11-non-fungible/tree/main/package.json#L62

<ins>Recommended Mitigation Steps</ins>

Update OpenZeppelin Contracts Usage in package.json

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Critical Changes Should Use Two-step Procedure

The critical procedures should be two step process.

See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure

<ins>Proof Of Concept</ins>

323: function setExecutionDelegate(IExecutionDelegate _executionDelegate)

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L323

332: function setPolicyManager(IPolicyManager _policyManager)

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L332

341: function setOracle(address _oracle)

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L341

350: function setBlockRange(uint256 _blockRange)

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L350

<ins>Recommended Mitigation Steps</ins>

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Public Functions Not Called By The Contract Should Be Declared External Instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

<ins>Proof Of Concept</ins>

function cancelOrder(Order calldata order) public {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L289

function withdraw(uint256 amount) public {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L44

function transferFrom(address from, address to, uint256 amount)
        public
        returns (bool)
    {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L58

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> require() / revert() Statements Should Have Descriptive Reason Strings

<ins>Proof Of Concept</ins>

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

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L240

291: require(msg.sender == order.trader);

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L291

573: require(remainingETH >= price);

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L573

45: require(_balances[msg.sender] >= amount);

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L45

48: require(success);

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L48

71: require(_balances[from] >= amount);

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L71

72: require(to != address(0));

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L72

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Non-usage of specific imports

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

<ins>Proof Of Concept</ins>

8: import "./lib/ReentrancyGuarded.sol";
9: import "./lib/EIP712.sol";
10: import "./lib/MerkleVerifier.sol";
11: import "./interfaces/IExchange.sol";
12: import "./interfaces/IPool.sol";
13: import "./interfaces/IExecutionDelegate.sol";
14: import "./interfaces/IPolicyManager.sol";
15: import "./interfaces/IMatchingPolicy.sol";

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L8-L15

7: import "./interfaces/IPool.sol";

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L7

<ins>Recommended Mitigation Steps</ins>

Use specific imports syntax per solidity docs recommendation.

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Open TODOs

An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.

<ins>Proof Of Concept</ins>

18: // TODO: set proper address before deployment

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L18

Summary<a name="Summary">

Gas Optimizations

Total: 33 contexts over 10 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> Using Private Rather Than Public For Constants, Saves Gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

<ins>Proof Of Concept</ins>

70: string public constant NAME = "Exchange";
71: string public constant VERSION = "1.0";
72: uint256 public constant INVERSE_BASIS_POINT = 10_000;
73: address public constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
74: address public constant POOL = 0xF66CfDf074D2FFD6A4037be3A669Ed04380Aef2B;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L70-L74

<ins>Recommended Mitigation Steps</ins>

Set variable to private.

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> Empty Blocks Should Be Removed Or Emit Something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

<ins>Proof Of Concept</ins>

66: function _authorizeUpgrade(address) internal override onlyOwner {}

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L66

15: function _authorizeUpgrade(address) internal override onlyOwner {}

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L15

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

<ins>Proof Of Concept</ins>

184: for (uint8 i = 0; i < executionsLength; i++) {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L184

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

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L307

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

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L598

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas

<ins>Proof Of Concept</ins>

295: require(!cancelledOrFilled[hash], "Order already cancelled or filled");

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L295

414: require(block.number - order.blockNumber < blockRange, "Signed block number out of range");

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L414

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

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L604

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> Use assembly to check for address(0)

Save 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
	if iszero(_addr) {
		mstore(0x00, "AddressZero")
		revert(0x00, 0x20)
	}
}

<ins>Proof Of Concept</ins>

function setOracle(address _oracle)
        external
        onlyOwner
    {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L341

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>

function cancelOrder(Order calldata order) public {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L289

function deposit() public payable {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L35

function withdraw(uint256 amount) public {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L44

function balanceOf(address user) public view returns (uint256) {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L79

function totalSupply() public view returns (uint256) {

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol#L83

<a href="#Summary">[GAS‑7]</a><a name="GAS&#x2011;7"> Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

<ins>Proof Of Concept</ins>

479: uint8 v;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L479

499: uint8 _v, bytes32 _r, bytes32 _s;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L499

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

<ins>Proof Of Concept</ins>

File: \2022-11-non-fungible\contracts\Exchange.sol

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol

File: \2022-11-non-fungible\contracts\Pool.sol

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Pool.sol

<a href="#Summary">[GAS‑9]</a><a name="GAS&#x2011;9"> Use uint256(1)/uint256(2) instead for true and false boolean states

If you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.

<ins>Proof Of Concept</ins>

42: isInternal = true;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L42

45: isInternal = false;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L45

146: isInternal = false;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L146

85: mapping(bytes32 => bool) public cancelledOrFilled;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L85

146: bool public isInternal = false;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L146

254: cancelledOrFilled[sellHash] = true;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L254

255: cancelledOrFilled[buyHash] = true;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L255

298: cancelledOrFilled[hash] = true;

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L298

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Using fixed bytes is cheaper than using 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.

<ins>Proof Of Concept</ins>

70: string public constant NAME = "Exchange";
71: string public constant VERSION = "1.0";

https://github.com/code-423n4/2022-11-non-fungible/tree/main/contracts/Exchange.sol#L70-L71