-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat: rely call detector [PoC] #382
base: master
Are you sure you want to change the base?
Conversation
// the crafter will then need to enable the following test and declare all rely calls | ||
// either this or the following tests must be run | ||
// cannot join 2 tests because this one requires `testFail` and the following `test` | ||
vm.expectEmit(false, false, false, false); |
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.
Shouldn't we validate at least topic 1 here?
vm.expectEmit(false, false, false, false); | |
vm.expectEmit(true, false, false, false); |
This way, it would only fail if rely
is called somewhere, otherwise, I guess any LogNote
would trigger a failure.
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.
Also we do have some contracts that use proper Rely(address)
events instead of LibNote, so this test would have to cover that as well.
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.
another test needs to cover it, same structure, only expecting emit a Rely(address)
(we cannot put both in the same test), as the expectation would only fail if BOTH the expectations are fulfilled
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 1st topic is ALWAYS validated, the 2nd topic (which corresponds to the 1st boolean) would be:
- for
Note
:address indexed usr
- for
Rely(address)
:address
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.
otherwise, I guess any LogNote would trigger a failure
beware that LogNote
is an anonymous
event, meaning, it does NOT have a topic 0 == keccak(LogNote)
, the topic0
IS the bytes4 indexed sig
Description
The following PoC shows a way of detecting specific types of calls, focusing on probably the most critical:
rely(address)
. Considering that if a wrong address is given throughout the execution of the spell, the whole protocol could be at risk (same as if an authorization is denied). This PoC aims to provide the process some tools to review that the spell is acting accordingly.Considering only
rely(address)
, there are 2 possible scenarios: either the spell needs to make one of these calls (and give authorization to an address), or the spell does not. The testing suite then incorporates 2 tests, one for each of the possible scenarios:testFailAuthCall
: will ✅ only if NOrely(address)
calls were made in spell executiontestAuthCall
: requires the crafter to declare all expectedrely(address)
calls (will ✅ if ALL of these calls are made)Taking into account that an alien auth call could be made in the second scenario (when we expect to see a rely event), the reviewer checklist should add something like:
In this way, adding an automated way of testing that there are no new authorizations, and a manual way of checking them when there are some, we can entrust that no new unwanted authorization is made.
Considerations:
rely(address)
comes from the legacy Note library, the test should be duplicated with newRely(address)
eventdeny(address)
andDeny(address)