_isValidSignature in DefaultAccount.sol is not compliant to EIP1271 as stated in the documentation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L161-L191

Vulnerability details

Impact

It is stated in the documentation on the ZkSync website and also in the NatSpec of the _isValidSignature that the validation should follow the EIP1271 standard but that is not true.

Proof of Concept

As you can see, in the documentation of ZkSync there is the section where it talks about Accounts Abstraction
https://era.zksync.io/docs/reference/concepts/account-abstraction.html
section where it speaks about the EIP1271 which is highly encourage to be used in those Accounts, since it is the standard that is endorsed by the ZkSync team
https://era.zksync.io/docs/reference/concepts/account-abstraction.html#eip1271
There are also comments in the NatSpec of DefaultAccount.sol contract which states that if the signature is valid it should return EIP1271_SUCCESS_RETURN_VALUE, otherwise reverts
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L160
In reality the function _isValidSignature is not compliant to EIP1271 as it is stated and does not return the value which is should. If we look closely to EIP1271 standard we can see two important things which does not happen in the implementation found in DefaultAccount.sol :

• firstly, per EIP1271 standard, the fuction MUST return the bytes4 magic value of 0x1626ba7e which represents the function signature of isValidSignature(bytes32,bytes) , as you can se here https://eips.ethereum.org/EIPS/eip-1271. In the case of DefaultAccount.sol the return value is not the correct function signature, as it should, since in case of a valid signature the magic value would be ACCOUNT_VALIDATION_SUCCESS_MAGIC https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L102-L103 which is equal to the selector of validateTransaction function as you can see here https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/interfaces/IAccount.sol#L7 selector which equates with 0x202bcce7 , different than the one imposed by the EIP1271
• secondly, it is stated by the EIP1271 that the function _isValidSignature MUST allow external calls https://eips.ethereum.org/EIPS/eip-1271, which in the case of DefaultAccount.sol it is not true since the function it is internal https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L161

Because of those issues the implementation is not EIP1271 compliant, even though it is stated multiple times in the documentation and in the NatSpec, that it is highly encouraged to use the EIP1271 since that is the standard endorsed by the ZkSync team, which could lead to multiple compatibility issues in the long run.

Tools Used

Manual review

Recommended Mitigation Steps

The first problem can be easily solved, since instead of returning the selector of validateTransaction which equates to 0x202bcce7, return the correct value of the EIP1271 which is the isValidSignature(bytes32,bytes) selector that equates to 0x1626ba7e. The second issue is more complicated to be solved, since you intend to have the DefaultAccount.sol behave like a EOA account, which means that any calls should be successful and return(0,0), which will not be the case if you make the function public and callable by anyone. What I believe should be done is documenting the difference implementation of EIP1271 in the DefaultAccount.sol, in case you want to keep the function internal, since nowhere is spoken about that and people could assume full compliancy with the standard, which is not true.

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[miladpiri]

QA. This report said more about inconsistency between documentation than any security issues.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) acknowledged

[GalloDaSballo]

Looks off, will double check:
https://eips.ethereum.org/EIPS/eip-1271

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Agree with the sponsor, in this case the comment is wrong

The calling function returns the MAGIC_VALUE which is compliant with the EIP

[GalloDaSballo]

@VagnerAndrei26 please refrain from making comments before PJQA

[VagnerAndrei26]

I want to address this issue and give some more explanations since I focused too much on the NatSpec and documentation in the reports. This is not only a comment issue, where the NatSpec/comments are wrong, this is a compliance issue, which most of the time is treated as a Medium on Code4Arena. I specified about documentation and NatSpec, to solidify my arguments about those specific functionalities needing to be EIP1271 compliant. As I said in my report, the function does not return the right value, as the EIP1271 suggests. The return value MUST BE 0x1626ba7e which is the function signature of isValidSignature(bytes32,bytes) , not any MAGIC_VALUE, and as I proved in my report the value returned is the function signature of validateTransaction(bytes32,bytes32,Transaction) which returns 0x202bcce7. This is not just a comment or documentation issue, because any future or external integration that would work with DefaultAccount, could assume EIP1271 compliance when signature gets validated, since it is talking in the documentation about it, and could expect the return value to be 0x1626ba7e, as stated by the EIP, but in reality the return magic value is 0x202bcce7, which again is not compliant. You can even see that in the very simple example provided in the EIP1271 of how the return value must look, if the signature is valid

 /**
   * @notice Verifies that the signer is the owner of the signing contract.
   */
  function isValidSignature(
    bytes32 _hash,
    bytes calldata _signature
  ) external override view returns (bytes4) {
    // Validate signatures
    if (recoverSigner(_hash, _signature) == owner) {
      return 0x1626ba7e;
    } else {
      return 0xffffffff;
    }
  }

Considering all of this, I believe this should not be treated as a simple NatSpec/Documentation issue, since it is about EIP compliancy and not just some wrong comments.

[GalloDaSballo]

@VagnerAndrei26 the function is not isValidSignature but rather validateTransaction

zkSync seems to be using the Validation Scheme from the EIP, but they don't seem to be implementing the EIP
Where is the proof / statement that zkSync uses the EIP?

[VagnerAndrei26]

Hey @GalloDaSballo , thanks for you time again. The main function used is validateTransaction, as you said, but this function still uses isValidSignature and EIP1271 for the main way of signature validation. The proof/statement of zkSync using the EIP is firstly in the NatSpec of the function where it says that the return value is intended to be EIP1271_SUCCESS_RETURN_VALUE
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L160C1-L160C1
, which again is not any random value, or abstract function signature, but specifically 0x1626ba7e, as per EIP, secondly the Account abstraction section of zkSync documentation, where they clearly speak about EIP1271
https://era.zksync.io/docs/reference/concepts/account-abstraction.html#eip1271.
And thirdly they also speak about it in the Signature Validation section of the Account abtraction, as you can see here
https://era.zksync.io/docs/reference/concepts/account-abstraction.html#signature-validation
where it clearly says that: We expect accounts to support the EIP-1271 standard , which again, it is not the case, since the whole base of EIP1271 standard is returning that specific value if the signature is correct, since it is a MUST rule, not a SHOULD , RECOMMENDED, MAY or OPTIONAL rule.
You specified about the fact that zkSync seems to implement the Validation Scheme of the EIP without intending to implement the EIP itself, but I argue this by saying that the Validation Scheme is the EIP itself, you can't speak about the EIP integration and implement the Validation Scheme, without implementing the EIP itself. As per whole, the EIP1271 intends to make validation of signature standardized, and instead of everyone implementing their return values, when the signature is correct, the same return value would be returned every time, which must be 0x1626ba7e. I understand that this can be argued by the sponsors, but nowhere, nor in the documentation or NatSpec, it is said that it should be or that it is a modified version of EIP1271 standard. Everywhere it speaks about the whole standard, with hyperlinks to the standard. So again, I do not consider that this is just some commenting issue, since there are multiple clear instances in NatSpec/Documentation of EIP1271 integration. Personally I assume that the developers believed that following that ecrecover scheme and return any bytes4 value, would make the integration EIP1271 compliant, but that is not the case.

[GalloDaSballo]

After spending quite some time reviewing this, I believe that the interpretation of the issues is incorrect and that QA severity is most appropriate

The finding asserts that zkSync is implementing EIP-1271 whereas my determination is that they are using EIP-1271 Signature Verification which is an idea from the EIP, and not the EIP itself

More specifically, the zkSync contracts are using EIP-1271 like signature verification

Changing the return value would cause more harm than good as it would brick previous contracts
All integrators would be made aware after the first tx or the first test
The behaviour discrepancy doesn't cause a loss of value

Overall I think this was a welcome suggestion, but is logically equivalent to my discussion here: code-423n4/org#130

[VagnerAndrei26]

After spending quite some time reviewing this, I believe that the interpretation of the issues is incorrect and that QA severity is most appropriate

The finding asserts that zkSync is implementing EIP-1271 whereas my determination is that they are using EIP-1271 Signature Verification which is an idea from the EIP, and not the EIP itself

More specifically, the zkSync contracts are using EIP-1271 like signature verification

Changing the return value would cause more harm than good as it would brick previous contracts All integrators would be made aware after the first tx or the first test The behaviour discrepancy doesn't cause a loss of value

Overall I think this was a welcome suggestion, but is logically equivalent to my discussion here: code-423n4/org#130

I understand and respect the final decision

[itsmetechjay]

Per the judge's request, marking as grade-C and closing.