Address Slither Issues in GoodDollar/Bancor Contracts #526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
setExitContribution, createExchange, destroyExchange are now onlyAvatar instead of onlyOwner for the GoodDollarExchangeProvider
i love the alphabet
GoodDollarExpansionController.setDistributionHelper is now onlyAvatar instead of onlyOwner
Base automatically changed from
feat/change-gdollar-modifiers
to
feat/GDollar-of-latest-develop
October 14, 2024 10:28
baroooo
reviewed
Oct 14, 2024
There was a problem hiding this comment.
looks good, but I don't think ReentrancyGuard provides any value here since the functions:
1- do not give the right of execution to any arbitrary contract, calls are made to known erc20 tokens and mento protocol contracts
2- follows check-effects-interactions pattern correctly
chapati23
force-pushed
the
chore/address-slither-issues
branch
3 times, most recently
from
2043749 to
cf48d9a
Compare
they didn't add any value as the mint functions only interact with trusted contracts and tokens
chapati23
force-pushed
the
chore/address-slither-issues
branch
from
cf48d9a to
99bd07c
Compare
chapati23
merged commit 914ba7d
into
feat/GDollar-of-latest-develop
4 of 5 checks passed
bowd
pushed a commit
that referenced
this pull request
Oct 24, 2024
7704a9b chore: reserve storage gap in new contracts 040b7fe GoodDollar Fork Tests (#530) 2ee14b4 Feat(454): Exchange provider tests (#529) 578747c test: add BancorExchangeProvider pricing tests (#532) fcd3d02 feat: improve tests around the expansion controller (#533) d3a31fa Streamline comments and errors (#528) ec64f0a fix: ☦ pray to the lords of echidna 13d3a98 fix: add back forge install to the CI step c236009 fix: echidna tests c50a198 feat: add Broker liquidity check (#523) 914ba7d Address Slither Issues in GoodDollar/Bancor Contracts (#526) 8bcf3fd Feat/change gdollar modifiers (#524) 379a511 feat: make validate() internal bcbd9aa chore: added "yarn todo" task to log out open todos in the code 0631c60 refactor: renamed SafeERC20 to SafeERC20MintableBurnable 2800297 feat: simplified SafeERC20 e858391 feat: extend open zeppelin's IERC20 instead of duplicating it 196f6be chore: fixed linter and compiler warnings 096efe2 test: fix fork integration test e5308d9 feat: add GoodDollar contracts + tests 3135626 feat: update Broker + TradingLimits to 0.8 and make G$ changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #525
Mostly false positives where I added comments.
One change I did make was adding reentrancy guards to the
mintfunctions in theGoodDollarExpansionController.Even though the slither warnings were only a "medium" around possibly manipulating event data via re-entrancy, I thought it'd still be a good idea to disallow reentrancy for all mint functions.