-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add the Xycloans case study #144
Conversation
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.
Amazing example! We should get some feedback from the Xycloans team on that
...AKERFYIKL/477912/entry-49ce70a0fe73f9b815f50dbe13403608242cec0770ac32eb24d1a425e39467be.json
Outdated
Show resolved
Hide resolved
const token = env.storage().instance().get('TokenId') | ||
|
||
// side-effects: we track deposits locally to compute rewards later | ||
this.shares = this.shares.update(from, 0n, (x) => x + amount) |
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 looks like a very fragile solution to me. What if we have two succeeds_with
conditions that both update this.shares
?
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.
We could move it outside the succeeds_with
. WDYT?
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.
yes, I believe it's how it should be done conceptually. Kind of like one update per tx.
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.
Uh, but here's the thing: we're passing Env
to succeeds_with
not to the containing method. However, the second side-effect below accesses storage via env
.
We're doing this because reverts_if
and succeeds_with
receive different Env
s: reverts_if
only has access to the pre-state, while succeeds_with
receives pre- and post-states.
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 will think about it a bit more, but maybe we need to leave this as a future design consideration.
1fe7f59
to
ce62543
Compare
Add a further case study, based on TypeScript monitors.
It reproduces a known and fixed criticial vulnerability exposed by a rounding error identified by OtterSec as finding
OS-XYC-ADV-00
in this audit report.Closes #133, closes #142