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

[CodeArena] - [L-19/L-11] : Add missing reserve checks and inputs length check #49

Merged
merged 1 commit into from
Dec 2, 2021
Merged

[CodeArena] - [L-19/L-11] : Add missing reserve checks and inputs length check #49

merged 1 commit into from
Dec 2, 2021

Conversation

maximebrugel
Copy link
Contributor

@maximebrugel maximebrugel commented Dec 2, 2021 •
edited
Loading

@maximebrugel maximebrugel added the To review Let people know this PR is ready for a review label Dec 2, 2021
adrien-supizet
adrien-supizet approved these changes Dec 2, 2021
View reviewed changes
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.

This particular check will be present 6 times in this file.
Should we consider using a modifier with an internal function to extract the check?
I don't feel strongly about it, I'm fine with repeating it

contracts/NestedFactory.sol Outdated

require(
nestedRecords.getAssetReserve(_nftId) == address(reserve),
"NestedFactory::addTokens: Assets in different reserve"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase and name the reason NF: RESERVE_MISMATCH

contracts/NestedFactory.sol Outdated
@@ -249,6 +252,10 @@ contract NestedFactory is INestedFactory, ReentrancyGuard, Ownable, MixinOperato
require(assetTokensLength > _tokenIndex, "NestedFactory::withdraw: Invalid token index");
// Use destroy instead if NFT has a single holding
require(assetTokensLength > 1, "NestedFactory::withdraw: Can't withdraw the last asset");
require(
nestedRecords.getAssetReserve(_nftId) == address(reserve),
"NestedFactory::withdraw: Assets in different reserve"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase and name the reason NF: RESERVE_MISMATCH

@adrien-supizet adrien-supizet added Can merge Good to go Fix and merge Minor changes required and removed To review Let people know this PR is ready for a review Can merge Good to go labels Dec 2, 2021
@maximebrugel
Copy link
Contributor Author

@adrien-supizet Fixed the require message + prettier

@maximebrugel maximebrugel merged commit e0528e9 into codearena Dec 2, 2021
@maximebrugel maximebrugel deleted the codearena-199 branch December 2, 2021 14:38
@adrien-supizet adrien-supizet changed the title [CodeArena] - Issue 199 : Add missing reserve checks [CodeArena] - Issue 103 && 199 : Add missing reserve checks and inputs length check Dec 2, 2021
@maximebrugel maximebrugel changed the title [CodeArena] - Issue 103 && 199 : Add missing reserve checks and inputs length check [CodeArena] - [L-19/L-11] : Add missing reserve checks and inputs length check Dec 8, 2021
maximebrugel added a commit that referenced this pull request Feb 16, 2022
@maximebrugel
fix: QA Report #49 (CodeArena)
a26c468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrien-supizet adrien-supizet adrien-supizet approved these changes

Assignees
No one assigned
Labels
Fix and merge Minor changes required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maximebrugel @adrien-supizet