Audit Review 06/07/2018

[manelli]

We discuss the changes introduced by the team in PR #26 as a response to our previous audit.

All the fixes will be added in PR #58

MultiMap.sol

• On insert, there is a check that a push to the array increases the length by 1.
  That is a correct safeguard, but if that fails it means that there is a very big bug on Solidity and we have bigger problems.
  Consider moving the check to the test suite.
  Fixed in: 4812c79 and e591b24

• Along the same lines, in the exist function, there is a superfluous check of the targetIndex being smaller than the length, which should be already covered by the condition on the right of the and operator (&&).
  Consider moving the first check to the test suite.
  WONTFIX: See: Audit Review 06/07/2018 #57 (comment)

DataOrder.sol

• The order has an initialBudgetForAudits, which is not used.
  Consider removing it.
  Fixed in: 097ac5c

• Consider renaming publicKey to buyerPublicKey.
  Fixed in: a4dd297

• Consider using hasSellerBeenAccepted in getSellerInfo and getNotaryForSeller, to avoid duplication.
  Fixed in: 8c39497

• There are duplicate checks in lines 122 and 123, as the orderStatus completed should be equivalent with transactionCompletedAt being nonzero.
  Consider removing one of these checks.
  Fixed in: fd56003

• As the getDataResponseStatusAsString default condition should never be reached, consider throwing instead of reverting.
  Fixed in: fe6ff3d
  Comment: the compiler issues the following warning: "throw" is deprecated in favour of "revert()", "require()" and "assert()".
  Fixed in: 5e48aeb

DataExchange.sol

• validNotaries is never used. Consider removing it.
  Fixed in: ab5175c

• setMinimumInitialBudgedForAudits can be called while paused.
  Consider adding the whenNotPaused modifier.
  Fixed in: 11fcfa7

• Consider renaming NotaryAdded to NotaryAddedToOrder , because the current name is too similar to NotaryRegistered.
  Fixed in: f0ac909

• It is not necessary to use SafeMath on allDistinct, because the maximum value is 5.
  Fixed in: b65b096

• chargeBuyer checks the allowance. This is duplicating the check in StandardToken.
  Fixed in: 093abd7

• Comment in payPlayers is wrong.
  It should say: // if notarization was done and data is invalid, order price tokens go back to buyer.
  Fixed in: 32f1871

• The TransactionCompleted event is emitted when DataResponse.status is TransactionCompleted or RefundedToBuyer.
  This could be misleading, consider adding an event for when the order is not completed but refunded.
  Fixed in: db00cf8

• The new payment logic is unclear, and there are inconsistencies between the code and the documentation.
  In particular, there seems to be a problem with the arguments of the min function.
  This min is not mentioned in the documentation, and it's likely that it is meant to avoid subtracting more than the available budget.
  Fixed in: 632e19e

General comments

• Consider renaming the notary argument to notaryAddress, to make it clear that it is an address and not an structure, name, or something else.
  WONTFIX: See Audit Review 06/07/2018 #57 (comment) and Audit Review 06/07/2018 #57 (comment)

• On many functions it is documented that the return value is “Whether [something] was successful or not.”
  This implies that false is returned when something goes wrong, but in most of the cases the function reverts instead.
  Consider making the docs clearer, saying something like: “returns true when [something] was successful. Reverts when it was not”.
  Ideally, specify all the cases that revert.
  Fixed in: 82313a9

Questions

• On DataExchange.chargeBuyer, the remainingBudget is only used to pay for the notarizationFee.
  It’s not clear why it isn't used to pay for the totalCharges.
  For example, if the notarizationFee is very close to 0, the remainingBudget will take a lot of time to be spent.
  Shouldn't notarizationFee be replaced by totalCharges in line 525?

• Wibcoin is not pausable. Should it be pausable too?

• What does it mean when dataHash is 0?

• What is the incentive for the buyer to add and close data responses?
  They could validate the signatures off-chain and avoid being charged.

[manelli]

@danielfx90 Regarding the following comment:

The TransactionCompleted event is emitted when DataResponse.status is TransactionCompleted or RefundedToBuyer.
This could be misleading, consider adding an event for when the order is not completed but refunded.

Should we emit a new event in the case the transaction was not completed or just change the event from TransactionCompleted to TransactionFinished (or anything that does not have similarities with DataResponseStatus.TransactionCompleted?

[danielfx90]

@manelli Let's add a new event and reflect exactly what happened with the data response.

[manelli]

Regarding the following comment:

Consider renaming the notary argument to notaryAddress, to make it clear that it is an address and not an structure, name, or something else.

I'm not sure if it will make things clear to use a Systems Hungarian-like notation. Moreover it seems like a code smell.
Also, it will make very difficult to read the diff, because most address parameters would have to be changed (order -> orderAddress, buyer, -> buyerAddress, etc)

@danielfx90 What do you think?

[manelli]

Regarding the following comment:

On many functions it is documented that the return value is “Whether [something] was successful or not.”
This implies that false is returned when something goes wrong, but in most of the cases the function reverts instead.
Consider making the docs clearer, saying something like: “returns true when [something] was successful. Reverts when it was not”.
Ideally, specify all the cases that revert.

We also should follow the Ethereum Natural Specification Format and use the correct tags. For example we are now using the @dev tag for what it should be under a @notice tag.

[danielfx90]

@manelli I agree with both of your comments.

For the first one, using types in variables doesn't sound right. But we should be consistent. I think we use orderAddr in same places, because we use order for the contract instance. Maybe we could make another distintion there for the contract and use order for the address.

As for the docstrings, go ahead!

[manelli]

The only functions in which the parameters are called orderAddr are the ones in which we instantiate a DataOrder from that address so it's probably ok to have it called like that.

[manelli]

Regarding the following comment:

Along the same lines, in the exist function, there is a superfluous check of the targetIndex being smaller than the length, which should be already covered by the condition on the right of the and operator (&&).
Consider moving the first check to the test suite.

Although the right hand check (self.addresses[targetIndex] == _key;) is sufficient to ensure the address exists, the left hand check (targetIndex < self.addresses.length exists to avoid out-of-bound errors.
Without the first check if we try to access an out-of bound element in the addresses array will result in an EVM exception.
This could happen when addresses[] is empty.

[danielfx90]

Audit done! Closing issue...