permit and `_isValidSignature` would fail to validate signatures from counterfactual wallets

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L195-L237

Vulnerability details

Impact

This vulnerability easily impacts Centrifuge's ability to validate signatures for counterfactual ERC-4337 accounts, limiting the usability for users of certain wallets that rely on AA.

Proof of Concept

EIP 1271 is being implemented within Centrifuge which allows contracts to sign messages and works great in tandem with EIP 4337 (account abstraction).

As we know, in ERC-4337, the account is not deployed until the first UserOp is mined. Before then, the account exists "counterfactually" — it has an address even though it's not really deployed. Usually, this is great since we can use the counterfactual address to receive assets without deploying the account first.

Do note that not being able to sign messages from counterfactual contracts/accounts has always been a limitation of ERC-1271 since one can't call the _isValidSignature() function on them.

Current method of validation of signatures checks if the signature's length is not 65 and then tries executing the _isValidSignature() as seen in ERC20.sol#L195-L237 which practically means that Centrifuge will fail to validate signatures for users of notorious wallets/projects, including Safe, Sequence, and Argent supporting ERC1271, but also ERC4337 wallets, even though a clear intention has been made to support signatures by EIP1271 compliant wallets, as confirmed by these tests.

Crux of the issue is the fact that protocol is taking responsibility to check the validity of signatures, but partially failing to trigger signature validation signatures for a group of wallets from a provider since (the validation will succeed if the ERC4337 wallet is deployed) and given that the protocol decided to support contract-based wallets (that support ERC4337) and implement ERC1271, one could assume that they "inherit" the possibility from the wallet providers to support ERC4337 too.

Tool used

Manual Review

Recommended Mitigation Steps

EIP-6492 solves this issue. The author of the EIP already implemented ERC-6492 in a universal library which verifies 6492, 712, and 1271 sigs with this pull request.

ERC6492 assumes that the signing contract will normally be a contract wallet, but it could be any contract that implements ERC-1271 and is deployed counterfactually.

• If the contract is deployed, produce a normal ERC-1271 signature
• If the contract is not deployed yet, wrap the signature as follows: concat(abi.encode((create2Factory, factoryCalldata, originalERC1271Signature), (address, bytes, bytes)), magicBytes)

Centrifuge could adopt the reference-implementation as stated in the EIP and delegate the responsibility of supporting counterfactual signatures to the wallets themselves, and this works because, as defined in the EIP, the wallet should return the magic value in both cases.

Additional Note

Justification for severity based on the Severity Categorization could be any where from Med to QA, with me siding on the Med side since availability of a crucial function is completely unavailable to some users, in this case counterfactual ERC-4337 accounts.

Lastly, a question was asked to sponsors during the course of the audit as can be seen here with a confirmation that both EOA and Contracts are allowed which factualizes the need for the above discussed issue to be rectified within protocol

Q: can users be contract ? or strictly EOA ? A: could be both

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #227

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #570

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[Bauchibred]

Hi @gzeon-c4,

I hope you're well. I noticed that this issue was marked as a duplicate to #570. While both issues pertain to potential complications arising from integrating counterfactual wallets, they address distinct concerns:

1. 570 talks about loss of funds.

2. This issue however, focuses on how validating signatures from counterfactual wallets wouldn't work.

In my view, the inability to validate signatures for certain users warrants a medium severity rating based on C4 rules, since this blocks off an important functionality of the protocol... quoting C4's severity categorization below.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Could you please review and clarify if otherwise? I appreciate your time and understanding. Thank you!

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[gzeon-c4]

Agree not a duplicate, but I believe this is QA due to it being a generic limitation of counterfactual wallets. @hieronx

[hieronx]

Agreed with @gzeon-c4 👍

[Bauchibred]

Thank you for your feedback, @gzeon-c4.

While I understand your perspective on this being a generic limitation of counterfactual wallets, I'd still argue this to not be QA, I believe the crux of the issue hinges on whether these wallets are officially supported by the protocol. If they are, this limitation indeed presents a significant concern. If not, having this clearly documented would have helped me not think about or even write a report on this attack window, but based on this comment and pending new information from sponsor, I tend to think that's not the case since sponsor only disputed said finding and not that counterfactual wallets are supported, no?

Furthermore, to support my claim, here is a previously considered finding on similar grounds that was confirmed as an issue, though would only be fair to note that this was concluded unsatisfactory due to the contract being out-of-scope.

I respect your judgment on this matter and look forward to any further insights or clarifications, particularly from sponsor. Thanks again for your time and consideration.

Edit: Was on the page for too long and @hieronx's comment didn't upload on my side.

[gzeon-c4]

Thank you for your feedback, @gzeon-c4.

While I understand your perspective on this being a generic limitation of counterfactual wallets, I'd still argue this to not be QA, I believe the crux of the issue hinges on whether these wallets are officially supported by the protocol. If they are, this limitation indeed presents a significant concern. If not, having this clearly documented would have helped me not think about or even write a report on this attack window, but based on this comment and pending new information from sponsor, I tend to think that's not the case since sponsor only disputed said finding and not that counterfactual wallets are supported, no?

Furthermore, to support my claim, here is a previously considered finding on similar grounds that was confirmed as an issue, though would only be fair to note that this was concluded unsatisfactory due to the contract being out-of-scope.

I respect your judgment on this matter and look forward to any further insights or clarifications, particularly from sponsor. Thanks again for your time and consideration.

Edit: Was on the page for too long and @hieronx's comment didn't upload on my side.

They are supported once they are deployed, it is hard to justify H/M for this particular edge case. Also worth to note EIP 6492 is finalized after the contest start date, and it is also not ideal to implement a draft EIP.

[Bauchibred]

Agree that implementing a draft EIP is not ideal, didn't look at it from that angle, completely understand your stance of this being QA, thanks for the additional insight.