Wrapper/Reentrancy unit tests, etc. #2042

dorothy-zbornak · 2019-08-08T00:12:05Z

Description

This PR introduces a couple things:

Exchange Wrapper Unit Tests

You'll find many new unit tests for Exchange wrapper functions in wrapper_unit_tests.ts.

This is accomplished by overriding a few Exchange internal functions in TestWrappersFunctions.sol so we test only the logic of the wrapper functions themselves.

Automatic Reentrancy Guard Testing

You'll find some lovely tests in reentrancy_tests.ts that automatically 🤖 checks if public Exchange functions are secured by the nonReentrant modifier. This voodoo is accomplished by the ReentrancyTester.sol contract.

By default, it will check all mutator public functions that are not whitelisted in test/utils/constants.ts, and it'll even check that those whitelisted functions are actually reentrant. This makes it much harder to accidentally expose a reentrant public function.

I've also removed a lot of the legacy reentrancy tests scattered around the tests because I feel like the new tests cover them. This is might be controversial. 😇

Other goodies:

marketBuyOrders and marketSellOrders have been removed and their noThrow variants have replaced them.
Signature validating functions in MixinSignatureValidator will now throw an InvalidSigner if the signerAddress == 0. This is to prevent people from using 0x0 as a signerAddress and passing bad signature data to ecrecover(), which will happily return 0x0.
There is no more assembly in fillOrderNoThrow 🎉
Updated the legacy Wallet signature type (which we still support in V3) to use the magic bytes pattern from v2.1 of the Exchange.
Added all mutator functions to constants.ExchangeFunctions in @0x/contracts/exchange/test/utils.ts.
Blacklisted yet another file from solhint because of abi.decode syntax: contracts/coordinator/contracts/src/MixinCoordinatorApprovalVerifier.sol. 😞

Some Thoughts

I think it might be cleaner to move ALL integration tests out of the exchange package and into its own package (like a protocol-tests package?), when we have more time. There's a lot of integration tests that pull in many external contract packages so things are looking a little spaghetti...

Testing instructions

You know the drill.

Types of changes

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2019-08-08T07:40:41Z

Coverage increased (+0.003%) to 78.686% when pulling ca33090 on feature/3.0/exchange/wrapper-tests into 8adfa52 on 3.0.

dorothy-zbornak · 2019-08-08T07:39:29Z

contracts/exchange-libs/contracts/src/LibExchangeSelectors.sol

@@ -22,9 +22,6 @@ pragma solidity ^0.5.9;
 contract LibExchangeSelectors {


I think we can probably ditch this file, right?

+1 the current approach is error-prone and not worth the small gas optimization over .selector.

Removed it entirely from the Exchange contracts. Coordinator and DevUtils still use it quite heavily, so I added it to the backlog.

hysz · 2019-08-08T16:20:47Z

I think it might be cleaner to move ALL integration tests out of the exchange package and into its own package.

I really like the idea of consolidating integration tests into a single package. We'd get an overview of protocol-wide test coverage without having to sift through all of our packages.

hysz

Lots of great stuff in this PR (~‾▿‾)~

hysz · 2019-08-08T16:39:33Z

contracts/exchange-libs/contracts/src/LibExchangeSelectors.sol

@@ -22,9 +22,6 @@ pragma solidity ^0.5.9;
 contract LibExchangeSelectors {


+1 the current approach is error-prone and not worth the small gas optimization over .selector.

contracts/exchange/contracts/test/ReentrancyTester.sol

hysz · 2019-08-08T17:33:09Z

contracts/exchange/contracts/test/ReentrancyTester.sol

+    /// Because this function uses the `nonReentrant` modifier, if
+    /// the function being called is also guarded by the `nonReentrant` modifier,
+    /// it will revert when it returns.
+    function testReentrantFunction(bytes calldata fnCallData)


Is this only called by isReentrant? If so, why not apply the nonReentrant modifier directly to isReentrant?

Mostly for UX reasons. Because our reentrancy guard is lazy, only second to the last non-reentrant call will revert. If I made isReentrant() nonReentrant, I wouldn't be able to capture and compare the revert data within the contract and provide a nice boolean result.

contracts/exchange/contracts/test/TestValidatorWallet.sol

hysz · 2019-08-08T17:51:51Z

contracts/exchange/test/wrapper_unit_tests.ts

@@ -0,0 +1,1129 @@
+import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs';


Readability lvl 💯

…nit_tests`.

…it_tests`.

`@0x/contracts-exchange`: Add more `wrapper_unit_tests` tests.

…computed for `wrapper_unit_tests`.

…idSigner`.

…match v2.1. `@0x/contracts-exchange`: Add EOA tests to `signature_validator`.

`@0x/contracts-exchange`: Remove old reentrancy tests. `@0x/contracts-exchange`: Remove `ReentrantERC20Token` contract.

`@0x/contracts-coordinator`: Add `MixinCoordinatorApprovalVerifier.sol` to `.solhintignore` because of `abi.decode` issues.

config.

`MarketXOrders' functions.

`@0x/contracts-test-utils`: Remove unecessary wait timeout in `LogDecoder`.

`@0x/contracts-exchange`: Remove references to `marketXOrdersNoThrow`.

`@0x/contracts-exchange`: Update reentrancy tests. `@0x/contracts-exchange`: Add all mutator functions to `ExchangeFunctions` type. `@0x/contracts-tes-utils`: Remove unused import.

… constant in `TestValidatorWallet.sol`. `@0x/contracts-exchange`: Remove references to `LibExchangeSelectors` in the Exchange.

abandeali1 · 2019-08-12T17:10:05Z

contracts/coordinator/.solhintignore

@@ -0,0 +1,2 @@
+# solhint can't parse `abi.decode` syntax.
+contracts/src/MixinCoordinatorApprovalVerifier.sol


Can we just disable that specific block of code? The rest of the contract is still being linted correctly right?

I tried but solhint throws a parser error when it hits that contract. I'm a little confused about why this just started happening and with only certain forms of abi.decode(). I don't think it's just my setup because the same thing happened on CI (that's how I first noticed it).

contracts/exchange/contracts/src/MixinSignatureValidator.sol

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

contracts/exchange/contracts/test/TestValidatorWallet.sol

abandeali1 · 2019-08-12T18:09:26Z

contracts/exchange/test/reentrancy_tests.ts

+
+import { constants as TestConstants } from './utils/constants';
+
+blockchainTests.resets('Reentrancy Tests', env => {


Nice! Having this all in one place is so much cleaner.

contracts/exchange/test/utils/types.ts

abandeali1 · 2019-08-12T18:15:21Z

contracts/exchange/test/wrapper_unit_tests.ts

@@ -0,0 +1,1129 @@
+import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs';


jalextowle

Looks good! This is much cleaner than reentrancy previously was. I had a few nits, but it looks good to go once those are addressed.

jalextowle · 2019-08-12T20:23:04Z

contracts/exchange/contracts/test/ReentrancyTester.sol

+    {}
+
+    /// @dev Calls a public function to check if it is reentrant.
+    function isReentrant(bytes calldata fnCallData)


I think that it would make sense to start compiling a list of functions that need this modifier so that we can manually check during code reviews. That would also make it really easy when we start using static analysis.

contracts/exchange/contracts/test/ReentrancyTester.sol

jalextowle · 2019-08-12T20:25:00Z

contracts/exchange/contracts/test/ReentrantERC20Token.sol

@@ -1,388 +0,0 @@
-/*


🔥 So glad we're getting rid of this contract

jalextowle · 2019-08-12T20:43:37Z

contracts/exchange/test/reentrancy_tests.ts

+
+import { constants as TestConstants } from './utils/constants';
+
+blockchainTests.resets('Reentrancy Tests', env => {


contracts/exchange/test/signature_validator.ts

…rom many `MixinSignatureValidator` functions.

…ncyTester.sol`.

…rNoThrow()`.

… return incorrect data.

…alidOrderSignature()` semantics.

dorothy-zbornak force-pushed the feature/3.0/exchange/wrapper-tests branch from 8169d02 to f9814b0 Compare August 8, 2019 07:24

dorothy-zbornak commented Aug 8, 2019

View reviewed changes

dorothy-zbornak requested review from abandeali1, jalextowle and hysz August 8, 2019 07:45

dorothy-zbornak marked this pull request as ready for review August 8, 2019 07:45

dorothy-zbornak requested a review from fabioberger as a code owner August 8, 2019 07:45

dorothy-zbornak changed the title ~~Wrapper/Reentrancy tests, etc.~~ Wrapper/Reentrancy unit tests, etc. Aug 8, 2019

dorothy-zbornak force-pushed the feature/3.0/exchange/wrapper-tests branch from f9814b0 to 7bc7090 Compare August 8, 2019 15:59

hysz approved these changes Aug 8, 2019

View reviewed changes

dorothy-zbornak force-pushed the feature/3.0/exchange/wrapper-tests branch from 7bc7090 to 5e9fe4f Compare August 10, 2019 01:27

dorothy-zbornak changed the base branch from feature/3.0/exchange/fill-order-unit-tests to 3.0 August 10, 2019 01:52

dorothy-zbornak force-pushed the 3.0 branch from 0739266 to 8adfa52 Compare August 10, 2019 02:07

dorothy-zbornak added 15 commits August 9, 2019 22:15

@0x/contracts: Create TestWrapperFunctions contract.

29a82f8

@0x/contracts-exchange: Add more tests to wrapper_unit_tests.

ab094ab

@0x/contracts-exchange: Add more tests to wrapper_unit_tests.

10a8291

@0x/contracts-exchange: Fix fillOrKillOrder() tests in `wrapper_u…

b73008d

…nit_tests`.

@0x/contracts-exchange: Add fillOrderNoThrow tests to `wrapper_un…

1f3f0dc

…it_tests`.

@0x/contracts-exchange: Add more tests to wrapper_unit_tests.

c18e8ba

@0x/contracts-exchange: Remove assembly from fillOrderNoThrow().

566e743

@0x/contracts-exchange: Make marketBuy/SellNoThrow the default.

ca28b8f

`@0x/contracts-exchange`: Add more `wrapper_unit_tests` tests.

@0x/contracts-exchange: Change how order hashes and signatures are …

ea1501a

…computed for `wrapper_unit_tests`.

@0x/contracts-exchange: Wrap up tests in wrapper_unit_tests.

7ce65e3

@0x/contracts-exchange: Update CHANGELOG, lint, prettify.

0d05411

@0x/order-utils: Add `ExchangeRevertErrors.SignatureErrorCode.Inval…

6752fc9

…idSigner`.

@0x/contracts-exchange: Update Wallet signature type behavior to …

3dd8dac

…match v2.1. `@0x/contracts-exchange`: Add EOA tests to `signature_validator`.

@0x/contracts-exchange: Create semi-automated reentrancy testing.

a2234b7

`@0x/contracts-exchange`: Remove old reentrancy tests. `@0x/contracts-exchange`: Remove `ReentrantERC20Token` contract.

@0x/contracts-exchange-libs: Regenerate selectors.

ae454b0

dorothy-zbornak added 11 commits August 9, 2019 22:15

@0x/contracts-coordinator: Update for marketXOrders name change.

1d0dce7

`@0x/contracts-coordinator`: Add `MixinCoordinatorApprovalVerifier.sol` to `.solhintignore` because of `abi.decode` issues.

Update changelogs.

cc8d5ac

@0x/contracts-exchange: Remove ReentrantERC20Token from compiler

b177882

config.

Update changelogs

921e78c

@0x/contracts-dev-utils: Remove references to 'NoThrow' variants of

5d54e6c

`MarketXOrders' functions.

@0x/contracts-exchange-libs: Update CHANGELOG.

14fe304

@0x/contracts-exchange: Fix broken tests.

c83864a

`@0x/contracts-test-utils`: Remove unecessary wait timeout in `LogDecoder`.

@0x/contracts-dev-util: Remove references to marketXOrdersNoThrow.

d4e300d

`@0x/contracts-exchange`: Remove references to `marketXOrdersNoThrow`.

@0x/contracts-dev-utils: Prettier.

370df0d

`@0x/contracts-exchange`: Update reentrancy tests. `@0x/contracts-exchange`: Add all mutator functions to `ExchangeFunctions` type. `@0x/contracts-tes-utils`: Remove unused import.

@0x/contracts-exchange-libs: Fix CHANGELOG rebase issues.

e34b390

@0x/contracts-exchange: Add comments to LEGACY_WALLET_MAGIC_VALUE…

ca33090

… constant in `TestValidatorWallet.sol`. `@0x/contracts-exchange`: Remove references to `LibExchangeSelectors` in the Exchange.

dorothy-zbornak force-pushed the feature/3.0/exchange/wrapper-tests branch from 5e9fe4f to ca33090 Compare August 10, 2019 02:15

abandeali1 suggested changes Aug 12, 2019

View reviewed changes

jalextowle approved these changes Aug 12, 2019

View reviewed changes

dorothy-zbornak added 7 commits August 12, 2019 17:30

@0x/contracts-exchange: Update IWallet for new legacy wallet spec.

a8ddbe4

@0x/contracts-exchange: Remove explicit signerAddress parameter f…

5b0b8a9

…rom many `MixinSignatureValidator` functions.

@0x/contracts-exchange: Light changes to address PR comments.

6df190e

@0x/contracts-exchange: Remove redundant new bytes(0) in `Reentra…

a3b2dbf

…ncyTester.sol`.

@0x/contracts-exchange: Fix bad resultData assertion in `fillOrde…

9382e2e

…rNoThrow()`.

@0x/contracts-exchange: Add tests for when callback signature types…

fc18db1

… return incorrect data.

@0x/contracts-dev-utils: Update OrderValidationUtils for new `isV…

98f7739

…alidOrderSignature()` semantics.

dorothy-zbornak requested a review from abandeali1 August 12, 2019 23:51

abandeali1 approved these changes Aug 13, 2019

View reviewed changes

dorothy-zbornak merged commit f22b03f into 3.0 Aug 13, 2019

dorothy-zbornak mentioned this pull request Aug 16, 2019

Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapper/Reentrancy unit tests, etc. #2042

Wrapper/Reentrancy unit tests, etc. #2042

dorothy-zbornak commented Aug 8, 2019 •

edited

Loading

coveralls commented Aug 8, 2019 •

edited

Loading

dorothy-zbornak Aug 8, 2019

hysz Aug 8, 2019

dorothy-zbornak Aug 10, 2019

hysz commented Aug 8, 2019

hysz left a comment •

edited

Loading

hysz Aug 8, 2019

hysz Aug 8, 2019

dorothy-zbornak Aug 8, 2019 •

edited

Loading

hysz Aug 8, 2019

abandeali1 Aug 12, 2019

abandeali1 Aug 12, 2019

dorothy-zbornak Aug 12, 2019

abandeali1 Aug 12, 2019

jalextowle Aug 12, 2019

abandeali1 Aug 12, 2019

jalextowle left a comment •

edited

Loading

jalextowle Aug 12, 2019

jalextowle Aug 12, 2019

jalextowle Aug 12, 2019

		@@ -22,9 +22,6 @@ pragma solidity ^0.5.9;
		contract LibExchangeSelectors {

		@@ -0,0 +1,1129 @@
		import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs';

		@@ -0,0 +1,2 @@
		# solhint can't parse `abi.decode` syntax.
		contracts/src/MixinCoordinatorApprovalVerifier.sol


		import { constants as TestConstants } from './utils/constants';

		blockchainTests.resets('Reentrancy Tests', env => {

Wrapper/Reentrancy unit tests, etc. #2042

Wrapper/Reentrancy unit tests, etc. #2042

Conversation

dorothy-zbornak commented Aug 8, 2019 • edited Loading

Description

Exchange Wrapper Unit Tests

Automatic Reentrancy Guard Testing

Other goodies:

Some Thoughts

Testing instructions

Types of changes

Checklist:

coveralls commented Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz commented Aug 8, 2019

hysz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalextowle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak commented Aug 8, 2019 •

edited

Loading

coveralls commented Aug 8, 2019 •

edited

Loading

hysz left a comment •

edited

Loading

dorothy-zbornak Aug 8, 2019 •

edited

Loading

jalextowle left a comment •

edited

Loading