Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ReentrancyGuard #503

Merged
merged 4 commits into from Apr 5, 2019

Conversation

Projects
None yet
5 participants
@sohkai
Copy link
Member

sohkai commented Apr 3, 2019

Adds a reusable ReentrancyGuard working off of its own unstructured storage slot, and makes it available to AragonApps.

See aragon/aragon-apps#744 (comment) for context.


Bytecode comparison:

                     CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
KeccakConstants.json 28400 more gas       +142               0

But the actual bytecode increase of adding the nonReentrant modifier is ~60k gas per use.

@sohkai sohkai requested review from izqui and facuspagnuolo Apr 3, 2019

@izqui

izqui approved these changes Apr 3, 2019

Copy link
Member

izqui left a comment

LGTM. Good job with the test coverage 💪

@@ -49,6 +49,7 @@ contract KeccakConstants {
// Unstructured storage
bytes32 public constant initializationBlockPosition = keccak256(abi.encodePacked("aragonOS.initializable.initializationBlock"));
bytes32 public constant depositablePosition = keccak256(abi.encodePacked("aragonOS.depositableStorage.depositable"));
bytes32 public constant reentrancyGuardPosition = keccak256(abi.encodePacked("aragonOS.reentrancyGuard.mutex"));

This comment has been minimized.

Copy link
@izqui

izqui Apr 3, 2019

Member

This is not being used for testing. Maybe we could use it in ReentrancyGuardMock so we implicitly test that it is correct.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 4, 2019

Author Member

Oops, forgot to add this to the keccak constants test. Add now in cf07f35.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 3, 2019

Coverage Status

Coverage increased (+0.004%) to 99.544% when pulling 6d7be74 on reentrancy-guard into dca0b4b on dev.

@bingen

bingen approved these changes Apr 3, 2019

Copy link
Member

bingen left a comment

This is really cool! It's going to make my life way happier! 😉

sohkai added some commits Apr 4, 2019

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

Great job @sohkai :)


it('allows entering non-entrant call from re-entrant call', async () => {
await reentrancyMock.reentrantCall(reentrantActor.address)
assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice')

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 5, 2019

Contributor

Just to ensure consistency I'd check that the mutex variable stays in false on successful calls when using the guard.

@sohkai sohkai force-pushed the reentrancy-guard branch from 070155c to 3cb559a Apr 5, 2019

@sohkai sohkai force-pushed the reentrancy-guard branch from 3cb559a to 6d7be74 Apr 5, 2019

@sohkai sohkai merged commit 6b56c93 into dev Apr 5, 2019

5 checks passed

License Compliance All checks passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 99.544%
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the reentrancy-guard branch Apr 5, 2019

sohkai added a commit that referenced this pull request Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.