Blur Exchange contest - trustindistrust's results

Non-critical

Events are confusingly named

Location

Description

The Pool contract uses events to log deposits and withdrawals. However, it uses the same event for both deposits and withdrawals. This makes log tracking on-chain harder to understand.

Suggested course of action

Create specific Deposit and Withdraw events for those actions.

Non-criticial

Events use incorrect value for parameter

Locations: 1 2

Description

The Pool contract uses events to log deposits and withdrawals. However, the code inserts the 0 address instead of the address of the contract, confusing the output.

Suggested course of action

Replace address(0) with address(this).

Non-Critical

Pool contract should not be upgradable.

Description

The Pool contract acts as a source of ETH funds for use in executing purchases on the Exchange. Users may deposit ETH and use that ETH for purchases, or withdraw it.

However, the contract is upgradable for no clear reason. It's simple in form and purpose. Since it holds user's ETH directly, it should be immutable and unable to defraud users later, either maliciously or by accident of management.

Suggested course of action

Remove all upgradable features from the Pool contract.

Non-critical: Move fee accumulator above the transfer of tokens in _transferFees

Location

While there is no risk of malicious effects, it is best practice to move changes to state or scoped state above side effects like transfers ("Checks Effects Interactions" pattern).

Non-critical: Remove return statement from _canMatchOrders as it is unnecessary

Location

_canMatchOrders uses named return values, and contains a branch that ensures that the named return values can't be shadowed. As such, the final return statement is unnecessary and can be removed.

Low: Sellers can grief by setting large Fee arrays containing 0 values, causing execute to consume more gas than expected to process trades.

Location

Description

When a sell order is created, it includes provision for a dynamic Fee array. This struct is designed to allow the seller to indicate the amount and destination for fees owed during a trade.

The loop that processes the trades in Exchange._transferFees() has a cap of 255 such fee entries. As such, a seller that wanted to grief buyers could create and sign a sell Order with an array of at least 256 members with the rate set to 0. While this would still result in a sum of 0 for fees, the dapp itself is unlikely to indicate the higher cost of gas to perform the trade.

Suggested course of action

If possible, modify the Order struct to contain a sized Fee array that is set to a reasonable value based on business case.

If that isn't possible, insert a check for 0 at line 599 that continues if the value is 0, thus reducing the gas impact of such an attack.

Gas: Remove extraneous computation from OrdersMatched event emission

Location

Unless there is a compelling business case for recording the maker/taker relationship of a trade, removing the computation for who was the maker and taker would save nearly 500 gas on a protocol hotpath. The effect is compounded if a bulk order is the source of the calls to _execute()

Consider instead simply recording the seller and buyer addresses. Since the inputs to _execute already contain the listingTime via the Order struct, the information can be computed offchain rather than forcing users to pay for the computation.