Store Custom Error Instance in LibraryCall #7
Conversation
|
This feature is to help https://github.com/CertiKProject/slither-task/pull/87. |
| @@ -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) | |||
There was a problem hiding this comment.
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 sOur 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That indeed is how the detector was written.
There was a problem hiding this comment.
Alright. I filed https://github.com/CertiKProject/slither-task/issues/94 to record the situation.
| @@ -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) | |||
There was a problem hiding this comment.
Alright. I filed https://github.com/CertiKProject/slither-task/issues/94 to record the situation.
This is similar to #3 , a library call to a custom error only stored the name of such custom error, but not its instance. This PR extends that feature by recording the instance.