Add Warnings in ERC-6492 Implementation

[pcaversaccio]

Summary

Since the exploit vector is publicly known, I consider the risk of discussing this matter openly to be low. Therefore, I'm opening a standard issue.

Last week a contract was exploited using the ERC-6492 reference implementation. An exemplary exploit contract can be retrieved here. So the major issue (apart from inheriting the implementation, which you should not do) is the combination of the identity precompile located at address 0x04 in combination with ERC-6492. Please note that there is an EIP proposal to replace the identity precompile with EVM code which we might can leverage in the future.

Actions to Discuss

• Add a warning that the universal verifier should not be inherited.
• We need to add a warning to the current ERC-6492 reference implementation regarding the usage of _signer = address(0x04) (and the combination with arbitrary calls). We could even go a step further, and either disallow that specific precompile address 0x04 or the full precompile address range: 0x00 - address(2**16-1).
• Do we see any impacts of the newly discovered attack vector for ERC-1271 implementations?

Cc: @Ivshti @Agusx1211

h/t goes to @0xkarmacoma for raising this issue with me.

[Agusx1211]

If I understand correctly, the exploit happened because "Odos Protocol" inherited from the UniversalSigValidator contract instead of deploying it as an independent contract and calling it. This is a bit of a footgun because the ERC does not explicitly mention that you DON'T need to inherit from the UniversalSigValidator or use it as a library internally.

I think we should perform two different mitigations:

1. Add a big warning that the UniversalSigValidator MUST NOT reside within any contracts of the protocol, and instead MUST be an external contract that is called when signatures need to be verified.

2. Modify the UniversalSigValidator so it is hard to inherit from it, maybe requesting a constructor argument that takes a string with the warning? It feels overkill, but it would force anyone to (1) remove the check or (2) copy-paste the text of the warning. Another option is to check the length of the contract itself at construction time.

3. Deploy the UniversalSigValidator ourselves using Nick's method (or some other universal factory).

[Agusx1211]

I think there may be a simpler solution for this issue. I was thinking that 4337 may be vulnerable in a similar way, but it isn't because it uses an intermediary "sender creator" that is created in the constructor of the entry point.

We can copy the same pattern, and inheriting from the UniversalSigValidator should become safe.

[Ivshti]

gm @Agusx1211 I already deployed it here: https://etherscan.io/address/0x7dd271fa79df3a5feb99f73bebfa4395b2e4f4be#code and it's via EIP-2470, so anyone can deploy it at the same addr.

@pcaversaccio @Agusx1211 The way I see it, this is a combo of the following issues

• anyone can spoof sig for 0x00..04
• if you inherit, you can make use of the arbitrary call that the validator does; this seems like a more serious footgun

Please confirm if my understanding is right

[Agusx1211]

Your understand is right, I am working on a pr to do the "call by proxy" pattern to remote the inherit footgun

[Agusx1211]

I am fairly sure this:

https://github.com/ethereum/ERCs/pull/878

Should be enough to stop the footgun, as inheriting from the contract would still create a proxy caller. Do you think we should still add a warning @Ivshti ?

[Agusx1211]

I am fairly sure ERC-1271 is vulnerable to the 0x00...04 attack too, since the magic value of ERC-1271 is the signature itself, any call to the identity precompile will have the return value looking like this:

0x1626ba7e...

Solidity would cast the result to bytes4, and the signature will be considered valid.

[Ivshti]

@Agusx1211 #878 feels like a bit of an overkill, I think comments in the code and in the ERC will be sufficient (and maybe a constructor).

However the more important issue is that we're not allowed to change ERCs for security reasons atm, last time I tried we had a discussion with @SamWilsn and we figured out the audit findings aren't sufficient to make any changes. Hopefully this will be.

[Agusx1211]

It does increase the cost of factory calls a little bit, but with such an easy to do footgun and the consequences being that you can drain the whole contract, I rather be safe than sorry here.

I do agree that we should at least be able to add a comment to the ERC @SamWilsn, both for 6492 and to 1271

[pcaversaccio]

Comments can be easily ignored and are no real footgun protection here. Reference implementations should be written in such a way that it is maximally hard to use them incorrectly (like any other smart contract library). Let's wait for feedback by @SamWilsn, but there is now a real case that emphasises how important it is to deal with this issue appropriately in both EIPs.

[0xkarmacoma]

I am fairly sure ERC-1271 is vulnerable to the 0x00...04 attack too, since the magic value of ERC-1271 is the signature itself, any call to the identity precompile will have the return value looking like this:

0x1626ba7e...

Solidity would cast the result to bytes4, and the signature will be considered valid.

The pattern used in the reference impl works because it does truncate the return value to bytes4:

      try IERC1271Wallet(_signer).isValidSignature(_hash, sigToValidate) returns (bytes4 magicValue) {
        bool isValid = magicValue == ERC1271_SUCCESS;

OpenZeppelin's SignatureChecker works differently, it checks that the output is at least 32 bytes and that the first 32 bytes decode to the magic value. But as a result, address(4) still looks like a valid signer regardless of the signature:

// returns true
SignatureChecker.isValidERC1271SignatureNow({signer: address(4), hash: bytes32(0), signature: new bytes(0)});

[Ivshti]

@0xkarmacoma @Agusx1211 @pcaversaccio EIP-1271 and ERc-6492 use the exact same way of verifying signatures. (6492 extends 1271), so if one is vulnerable so is the other. 6492 only adds the pre-deployment wrapper.

As for the warning, I suggest this: "// ⚠️ WARNING ⚠️ DO NOT INHERIT FROM THIS CONTRACT. This will allow arbitrary un-authorized calls from your contract." added to the contract itself. The likelihood of passing a bigger change is near zero.

[pcaversaccio]

I can see that they put it on the agenda for the EIPIP meeting on 19 February: https://github.com/ethcatherders/EIPIP/issues/378. Will you guys participate in that call? It's important to make them understand why the EIP requires an update (if it's a simple warning or better an adjusted reference implementation can be discussed in the next step).

[Ivshti]

I will join for sure @pcaversaccio