Rolla Order Protocol - Halborn Report Response

(HAL-01) LACK OF TRANSFEROWNERSHIP PATTERN

Transfer-accept ownership pattern implemented in commit 52ccfde

(HAL-02) THE CALLER CAN NOT SPECIFY THRESHOLD TO SPECIFY BOUNDS FOR THE TRADE SUCCEEDING

Since the trade is with a counterparty, and there is no scope for the order amount to change since it's submitted by the taker, the order will either fail or succeed depending on the taker amount passed as a parameter. This can be calculated up front without needing to pass in a slippage tolerance or any bounds. In the case of a partial fill, the amount can also be calculated up front.

(HAL-03) POSSIBLE ROUNDING ERROR CAN CAUSE TO ADDITIONAL ASSET TAKE OVER

Rounding is done to favor the maker. For fees, rounding is done to favor the user.

(HAL-04) USE OF ECRECOVER IS SUSCEPTIBLE TO SIGNATURE MALLEABILITY

Fixed in commit f9a4a47

(HAL-05) NO ERC20 SAFEAPPROVE VERSIONS CALLED

safeApprove added in commit 0ea0ba8

(HAL-06) THE CONTRACT SHOULD SAFEAPPROVE(0) FIRST

Approval will only be called once, and it's going to be set to the max uint value, so no need to approve 0 first.

(HAL-07) REMOVE UNNECESSARY RECEIVE FUNCTION

The receive function is required for the fillOrderWithChainToken method to work. Removing the receive function will cause functionality to break and all fillOrderWithChainToken tests to fail.

(HAL-08) LACK OF INPUT VALIDATION ON THE ORDER MAKER VARIABLE

The signature verification would fail if the order maker was the zero address, so there's no need to add another check here.

(HAL-09) MISSING ZERO-ADDRESS CHECK

Zero address checks were added to the constructor on commit c7d936c.

(HAL-10) INCOMPATIBILITY WITH INFLATIONARY TOKENS

We don't plan to support inflationary tokens in the forseeable future, and tokens need to be added to a curated AssetsRegistry contract first so that options can be created for them. So as long as the protocol multisig and a possible future governance contract don't add inflationary tokens to the AssetsRegistry, the RollaOrderProtocol should be safe from those possible issues. Additionally, for the maker asset no inflationary tokens will be used.

(HAL-11) FLOATING PRAGMA

We decided to leave floating pragmas for the interfaces. Version pragmas are locked in the implementation contracts.

(HAL-12) CONSTANT VARIABLES USING KECCAK CAN BE IMMUTABLE

Additional tests were written and run, and no gas savings was noticed when using immutable for the typehashes over constant. In fact, gas usage was lower when using constant.

(HAL-13) FRONT-RUNNING ATTACK ON INITIALIZATION FUNCTION

IController is just an interface. We don't have any upgradable proxy contracts in the rolla-order-protocol repo.

(HAL-14) MISSING RE-ENTRANCY PROTECTION

The relevant code already follows the checks-effects-interactions pattern, so there's no need for extra re-entrancy protection.

(HAL-15) USAGE OF BLOCK-TIMESTAMP

No change required.