Closed sam-goldman closed 2 months ago
This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?
Namely, I think we need to address the following scenario, which you mentioned to me a while ago:
- I create a merkle tree which includes a call to contract A to perform some check. I expect this call to revert if the check is does not pass.
- Some attacker executes the bundle on a network where A has not been deployed.
- The check does not revert because A does not exist.
- The attacker is able to execute the entire tree even though the check did not really pass.
Good catch - I added this to the SphinxModule
and provided an explanation for how I resolved this scenario below. Also, in this initial review, you don't need to go too deep into the security model. I'm just looking for the outcome of this initial review to be either:
I hope this is clear from your perspective. The main purpose of this initial review is to catch obviously incorrect design decisions before I spend a bunch of time testing, updating specs, etc.
I resolved the griefing scenario you mentioned by adding a new field to the EXECUTE
leaf, requireIsContract
. If this field is true
, we check that the to
field is a contract. Here's how this'd resolve the specific scenario you outlined, where Contract A must be deployed:
PreFlightCheck
, which will look something like this:
contract PreFlightCheck {
function check(bytes memory data) external {
(address target) = abi.decode(data, (address));
require(target.code.length > 0);
}
}
EXECUTE
leaf to be a call to PreFlightCheck
, and set requireIsContract
to true
.PreFlightCheck
hasn't been deployed yet, the SphinxModule
will revert because of the requireIsContract
check.PreFlightCheck
has been deployed but Contract A hasn't been deployed, the call to IsContract
will throw an error, causing the SphinxModule
to also throw an error.
This branch won't be merged anywhere. I'm just using it as an easy way to share design considerations for the upcoming audit.