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

[CodeArena] - Med/High Risk fixes #100

Merged
merged 8 commits into from
Feb 18, 2022
Merged

[CodeArena] - Med/High Risk fixes #100

merged 8 commits into from
Feb 18, 2022

Conversation

@maximebrugel maximebrugel added the To review Let people know this PR is ready for a review label Feb 16, 2022
Copy link
Contributor

@adrien-supizet adrien-supizet left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -486,6 +488,7 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, OwnableProxyDelegatio
bool _fromReserve
) private returns (IERC20, uint256) {
if (address(_inputToken) == ETH) {
require(!_fromReserve, "NF: NO_ETH_FROM_RESERVE");
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ I'm not sure I understand why is this one necessary.

Copy link
Contributor Author

@maximebrugel maximebrugel Feb 18, 2022

Choose a reason for hiding this comment

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

safe guard to not use ETH when it's from the reserve (this is possible) and the user can take the ETH from the contract

@adrien-supizet adrien-supizet added Can merge Good to go and removed To review Let people know this PR is ready for a review labels Feb 17, 2022
@maximebrugel maximebrugel merged commit 19441e3 into master Feb 18, 2022
@maximebrugel maximebrugel deleted the med-high-risks branch February 18, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can merge Good to go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants