-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: match delta p2pDisabled #1453
Conversation
c9e3439
to
1b2f7e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should point this PR to an upgrade-1
branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One additional test would be nice, but we can put that in an issue for now, as you wish
Why not btw ? We can put it in the fixes batch no ? |
9689dad
to
e194f11
Compare
e194f11
to
52c8f7e
Compare
52c8f7e
to
caed1e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Could we store the boolean p2pDisabled
as a variable as it's used twice?
1255971
to
1098a10
Compare
1098a10
to
50d5cd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions/suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been resolved without commit being pushed, perhaps you forget to push
Yes lol |
looks good to me. Just to clarify/document: The reason why we're only preventing this for |
@@ -251,11 +251,12 @@ contract PositionsManager is IPositionsManager, MatchingEngine { | |||
SupplyVars memory vars; | |||
vars.poolBorrowIndex = lastPoolIndexes[_poolToken].lastBorrowPoolIndex; | |||
vars.remainingToSupply = _amount; | |||
bool p2pDisabled = p2pDisabled[_poolToken]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local p2pDisabled
variable is shadowing the state variable p2pDisabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is common enough in the codebase. You think that this is an issue ?
For me the type system of Solidity does the job of preventing any confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this specifically is a problem, but if you can avoid it, it would be much better. For the same reason, I have created an information issue for all the other instances I've found about shadowing a state variable.
See https://swcregistry.io/docs/SWC-119 for more info about the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be better if avoided, but it should not be at the cost of adding new variable names for the same thing everywhere.
For next iterations, we should agree on a naming convention for stack and memory variables that removes any ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging it, for now, we'll discuss this point and find a general solution for this
Issue: when the peer-to-peer is disabled, anyone can match deltas (and so re-create new p2p). It should not be possible.
Fix: https://github.com/spearbit-audits/morpho-novemberAudit/issues/28