Blur Exchange contest - Aymen0909's results

QA Report

Summary

Findings

1- Front-runable initialize function :

The initialize function inside the Exchange contract is used to initialize the contract owner and important contract parameters (executionDelegate, policyManager, oracle), but any attacker can initialize the contract before the legitimate deployer and even if the developers when deploying call immediately the initialize function, malicious agents can trace the protocol deployment transactions and insert their own transaction between them and by doing so they front run the developers call and gain the ownership of the contract and set the wrong parameters.

The impact of this issue is :

• In the best case developers notice the problem and have to redeploy the contract and thus costing more gas.

• In the worst case the protocol continue to work with the wrong owner and state parameters which could lead to the loss of user funds.

Risk : Low

Proof of Concept

Instances include:

File: contracts/Exchange.sol Line 112-117

function initialize(
    IExecutionDelegate _executionDelegate,
    IPolicyManager _policyManager,
    address _oracle,
    uint _blockRange
) external initializer

Mitigation

It's recommended to use the constructor to initialize non-proxied contracts.

For initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify that the transaction succeeded.

2- Setters should check the input value and revert if it's the zero address or zero :

When setting a new value to a state variable the setter function must check the new value and revert if it's the zero address or zero..

Risk : Low

Proof of Concept

Instances include:

File: contracts/Exchange.sol Line 350-352

function setBlockRange(uint256 _blockRange) external onlyOwner

Mitigation

Add non-zero address/uint checks in the setters for the instances aforementioned.

3- require()/revert() statements should have descriptive reason strings :

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/Exchange.sol

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

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

require(remainingETH >= price);

File: contracts/Pool.sol

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

require(_balances[from] >= amount);

require(to != address(0));

Mitigation

Add reason strings to the aforementioned require statements for better comprehension.

4- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/Exchange.sol

returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

Gas Optimizations

Summary

Findings

1- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 4 instances of this issue:

File: contracts/Exchange.sol Line 574

remainingETH -= price;

The above operation cannot underflow due to the check require(remainingETH >= price); and should be modified as follows :

unchecked {remainingETH -= price;}

File: contracts/Exchange.sol Line 607

uint256 receiveAmount = price - totalFee;

The above operation cannot underflow due to the check require(totalFee <= price, "Total amount of fees are more than the price"); and should be modified as follows :

uint256 receiveAmount;
unchecked {receiveAmount = price - totalFee;}

File: contracts/Pool.sol Line 46

_balances[msg.sender] -= amount;

The above operation cannot underflow due to the check require(_balances[from] >= amount); and should be modified as follows :

unchecked {_balances[msg.sender] -= amount;}

File: contracts/Pool.sol Line 73

_balances[from] -= amount;

The above operation cannot underflow due to the check require(_balances[from] >= amount); and should be modified as follows :

unchecked {_balances[from] -= amount;}

2- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 6 instances of this issue:

File: contracts/Exchange.sol Line 316

nonces[msg.sender] += 1;

File: contracts/Exchange.sol Line 574

remainingETH -= price;

File: contracts/Pool.sol Line 36

_balances[msg.sender] += msg.value;

File: contracts/Pool.sol Line 46

_balances[msg.sender] -= amount

File: contracts/Pool.sol Line 73

_balances[from] -= amount;

File: contracts/Pool.sol Line 74

_balances[to] += amount;

3- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 3 instances of this issue:

File: contracts/Exchange.sol Line 184

for (uint8 i = 0; i < executionsLength; i++)

File: contracts/Exchange.sol Line 307

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

File: contracts/Exchange.sol Line 598

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

4- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 6 instances of this issue:

File: contracts/Exchange.sol

56       function open() external onlyOwner
60       function close() external onlyOwner
323      function setExecutionDelegate(IExecutionDelegate _executionDelegate) external onlyOwner
332      function setPolicyManager(IPolicyManager _policyManager) external onlyOwner
341      function setOracle(address _oracle) external onlyOwner
350      function setBlockRange(uint256 _blockRange) external onlyOwner