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.

Sign up for GitHub

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

Jump to bottom

Implement a reentrancy guard #236

Merged
merged 33 commits into from
Apr 1, 2022
Merged

Implement a reentrancy guard #236

merged 33 commits into from
Apr 1, 2022

Conversation

JulissaDantes
Copy link
Contributor

Fixes #228

Implements a reentrancy guard contract with 2 steps, start and end, that should be above and below the function body

martriay
martriay suggested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good! Left some comments. Also, I think we should also test that the protected function can be called under normal conditions.

openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_attacker_mock.cairo Outdated Show resolved Hide resolved
tests/security/test_reentrancy.py Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
docs/Security.md Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_attacker_mock.cairo Outdated Show resolved Hide resolved
andrew-fleming
andrew-fleming reviewed Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! I left some questions and comments. Also, I think we should add a test case testing that a protected function still executes properly.

docs/Security.md Show resolved Hide resolved
docs/Security.md Outdated Show resolved Hide resolved
openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_attacker_mock.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
tests/security/test_reentrancy.py Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
@JulissaDantes JulissaDantes changed the title Reentrancy guard Implement a reentrancy guard Mar 28, 2022
martriay
martriay suggested changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements! We're almost there.

docs/Security.md Outdated Show resolved Hide resolved
docs/Security.md Show resolved Hide resolved
openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
tests/security/test_reentrancy.py Outdated Show resolved Hide resolved
tests/mocks/reentrancy_mock.cairo Outdated Show resolved Hide resolved
andrew-fleming
andrew-fleming previously requested changes Mar 30, 2022
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements! Please ignore the storage var suggested change.

openzeppelin/security/reentrancy_guard.cairo Outdated Show resolved Hide resolved
@andrew-fleming andrew-fleming dismissed their stale review March 30, 2022 11:10

Requested change is not necessary

JulissaDantes and others added 19 commits March 31, 2022 09:07
@JulissaDantes
Add reentrancy contract
634e144
@JulissaDantes
Add test file amd rename contract
1bf2df8
@JulissaDantes
Add mock contracts for reentrancy
509b535
@JulissaDantes
Update mock test
e63458e
@JulissaDantes
Update reentrancy test
a77b26c
@JulissaDantes
Remove unused imports
2877244
@JulissaDantes
Update reentrancy tests
fa4d2af
@JulissaDantes
Add documentation
e2c3778
@JulissaDantes
Update Security.md
20e4e1e
@JulissaDantes
Add test cases
bae237b
@JulissaDantes
Remove attacker delegate call
da29aa9
@JulissaDantes @martriay
Update openzeppelin/security/reentrancy_guard.cairo
4964f2e 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update openzeppelin/security/reentrancy_guard.cairo
e300158 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update tests/mocks/reentrancy_attacker_mock.cairo
18e29f0 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update tests/mocks/reentrancy_mock.cairo
973ef38 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update docs/Security.md
b3a2635 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes
Fix variable names and use if instead of assert
0ad2d50
@JulissaDantes
Update name on reentrancy guard
bb459d0
@JulissaDantes @andrew-fleming
Update tests/mocks/reentrancy_mock.cairo
d8b48e4 
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
JulissaDantes and others added 12 commits March 31, 2022 09:08
@JulissaDantes @andrew-fleming
Update tests/mocks/reentrancy_mock.cairo
1822437 
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
@JulissaDantes
Add links on Security docs
c264801
@JulissaDantes
Remove alloc_locals
d206869
@JulissaDantes
Add initialize
9f60c19
@JulissaDantes
Remove unused function call
3d7a6ed
@JulissaDantes
Update reentrancy guard design
41113b1
@JulissaDantes
Update namespace information
9d82844
@JulissaDantes @martriay
Update docs/Security.md
d5760a3 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update tests/mocks/reentrancy_mock.cairo
c2de21e 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @martriay
Update tests/security/test_reentrancy.py
96c12a4 
Co-authored-by: Martín Triay <martriay@gmail.com>
@JulissaDantes @andrew-fleming
Update openzeppelin/security/reentrancy_guard.cairo
1c73c69 
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
@JulissaDantes
Update files location
02fa701
martriay
martriay approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

andrew-fleming
andrew-fleming approved these changes Apr 1, 2022
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

@martriay martriay merged commit 9cd6f0b into OpenZeppelin:main Apr 1, 2022
@andrew-fleming andrew-fleming mentioned this pull request Apr 4, 2022
Closed
@milancermak milancermak mentioned this pull request Apr 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frangio frangio frangio left review comments

@andrew-fleming andrew-fleming andrew-fleming approved these changes

@martriay martriay martriay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ReentrancyGuard
4 participants
@JulissaDantes @frangio @martriay @andrew-fleming