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

Store Custom Error Instance in LibraryCall #7

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Store Custom Error Instance in LibraryCall #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions slither/slithir/convert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,12 +774,18 @@ def propagate_types(ir, node: "Node"): # pylint: disable=too-many-locals
raise SlithIRError(f"Not handling {type(ir)} during type propagation")
return None

def resolve_event(event_name, event_collection):
for event in event_collection.events:
def resolve_event(event_name : str, c : Contract) -> Optional["Event"]:
for event in c.events:
if event.name == event_name:
return event
return None

def resolve_error(error_name : str, c : Contract) -> Optional["CustomError"]:
for ce in c.custom_errors:
if ce.name == error_name:
return ce
return None

def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]): # pylint: disable=too-many-locals
assert isinstance(ins, TmpCall)
if isinstance(ins.called, Variable) and isinstance(ins.called.type, FunctionType):
Expand Down Expand Up @@ -837,6 +843,9 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]): # pylint: dis
ins.lvalue,
ins.type_call,
)
custom_error_sym = resolve_error(str(ins.ori.variable_right), ins.ori.variable_left)
Copy link

Choose a reason for hiding this comment

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

On line 996, there is a CustomError case handler (presumably for top-level custom error calls).

    if isinstance(ins.called, CustomError):
        sol_function = SolidityCustomRevert(ins.called)
        s = SolidityCall(sol_function, ins.nbr_arguments, ins.lvalue, ins.type_call)
        s.set_expression(ins.expression)
        return s

Our implementation should reflect the top-level path's CustomError handling.
(Though, it's annoying that they are still using SolidityCall, instead of creating CustomErrorCall there.)

Also, I'd like this code to be more symmetric to the EventCall case above.
Let's create a separate if-statement here just like the event_sym leaving this default case as the normal library call.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Author

Choose a reason for hiding this comment

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

Upon thinking about this: the thing is we don't have a CustomErrorCall, let's assume for a moment that we don't want to implement that. Then the following scenario:

library L {
    error E();
}

contract C {
    function test() {
        revert L.E();
    }
}

the revert L.E() would be converted into a LibraryCall, which is handled on line 846. In this case I have to make the changes as I made in this PR. The counterpart for event (event emit through library, not normal event emission. Normal event emission is handled around line 804) is also handled around there. Similarly, line 996 which you pointed out handled only emission of top level custom error and in-contract custom error, which is not the issue I want to address through this PR.

The easy and temporary solution would be as is as of this PR. If we want to refactor it to be more scalable, the way to do is implement CustomErrorCall.

Copy link
Author

Choose a reason for hiding this comment

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

I guess I should clarify: the normal custom error revert already stored the instance of its custom error, this PR specifically want to address the case where a custom error from library is reverted.

Copy link

Choose a reason for hiding this comment

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

Ok, let's keep it a LibraryCall when a library error is constructed :(
So, our detector will have to deal with both LibraryCall and SolidityCall.

Copy link
Author

Choose a reason for hiding this comment

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

That indeed is how the detector was written.

Copy link

Choose a reason for hiding this comment

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

Alright. I filed https://github.com/CertiKProject/slither-task/issues/94 to record the situation.

if custom_error_sym is not None:
libcall.function = custom_error_sym
libcall.set_expression(ins.expression)
libcall.call_id = ins.call_id
return libcall
Expand Down