executeTransactionFromOutside incorrectly validates the passed _transaction

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L134 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L158-L192 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L74-L110

Vulnerability details

Impact

Invalid transactions (with incorrect signatures) can be executed through DefaultAccount.executeTransactionFromOutside.

Proof of Concept

DefaultAccount._isValidSignature uses ecrecover and checks if the recovered signer address is the same as the account's address. In case it is not, it either reverts or returns false:

  158:    /// @notice Validation that the ECDSA signature of the transaction is correct.
  159:    /// @param _hash The hash of the transaction to be signed.
  160:    /// @param _signature The signature of the transaction.
  161:    /// @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It reverts otherwise. // @audit wrong comment
  162:    function _isValidSignature(bytes32 _hash, bytes memory _signature) internal view returns (bool) {
  ...:        ...
  188:
  189:        address recoveredAddress = ecrecover(_hash, v, r, s);
  190:
  191:        return recoveredAddress == address(this) && recoveredAddress != address(0);
  192:    }

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L158-L192

The boolean returned above is later checked in DefaultAccount._validateTransaction and converted to a magic word of type bytes4:

   74:    /// @notice Inner method for validating transaction and increasing the nonce
   75:    /// @param _suggestedSignedHash The hash of the transaction signed by the EOA
   76:    /// @param _transaction The transaction.
   77:    function _validateTransaction(
   78:        bytes32 _suggestedSignedHash,
   79:        Transaction calldata _transaction
   80:    ) internal returns (bytes4 magic) { // @audit missing NatSpec @return comment
  ...:        ...
  104:
  105:        if (_isValidSignature(txHash, _transaction.signature)) {
  106:            magic = ACCOUNT_VALIDATION_SUCCESS_MAGIC;
  107:        } else {
  108:            magic = bytes4(0);
  109:        }
  110:    }

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L74-L110

_validateTransaction method is used in two places - in DefaultAccount.validateTransaction and DefaultAccount.executeTransactionFromOutside:

   66:    function validateTransaction(
   67:        bytes32, // _txHash
   68:        bytes32 _suggestedSignedHash,
   69:        Transaction calldata _transaction
   70:    ) external payable override ignoreNonBootloader ignoreInDelegateCall returns (bytes4 magic) {
   71:        magic = _validateTransaction(_suggestedSignedHash, _transaction);
   72:    }

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L66-L72

  130:    function executeTransactionFromOutside(
  131:        Transaction calldata _transaction
  132:    ) external payable override ignoreNonBootloader ignoreInDelegateCall {
  133:        // The account recalculate the hash on its own
  134:        _validateTransaction(bytes32(0), _transaction);
  135:        _execute(_transaction);
  136:    }

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L130-L136

As you can see, the magic word is returned to the bootloader in DefaultAccount.validateTransaction so it can be verified there. But this is not the case in DefaultAccount.executeTransactionFromOutside where if the signature does not correspond to the account address or the account address is the null address, the validation will silently fail and the _transaction will be executed.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following check in DefaultAccount.executeTransactionFromOutside:

  130:    function executeTransactionFromOutside(
  131:        Transaction calldata _transaction
  132:    ) external payable override ignoreNonBootloader ignoreInDelegateCall {
  133:        // The account recalculate the hash on its own
- 134:      _validateTransaction(bytes32(0), _transaction)
+ 134:      if(_validateTransaction(bytes32(0), _transaction) != ACCOUNT_VALIDATION_SUCCESS_MAGIC) revert();
  135:        _execute(_transaction);
  136:    }

Also, correct the NatSpec @return comment on DefaultAccount._isValidSignature:

  158:    /// @notice Validation that the ECDSA signature of the transaction is correct.
  159:    /// @param _hash The hash of the transaction to be signed.
  160:    /// @param _signature The signature of the transaction.
- 161:    /// @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It reverts otherwise.
+ 161:    /// @return true if the signaure is correct, false or reverts otherwise.
  162:    function _isValidSignature(bytes32 _hash, bytes memory _signature) internal view returns (bool) {

[c4-judge]

GalloDaSballo marked the issue as duplicate of #30

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-c