zkSync Era System Contracts contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L232-L247

Vulnerability details

Impact

may be missing markAccountCodeHashAsConstructed(), this causes the code corresponding to the address to be invalid

Proof of Concept

forceDeployOnAddress() can be used to forcefully deploy a contract. The code is as follows:

    function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {
        _ensureBytecodeIsKnown(_deployment.bytecodeHash);
        _storeConstructingByteCodeHashOnAddress(_deployment.newAddress, _deployment.bytecodeHash); //<-----set codehash to Constructing

        AccountInfo memory newAccountInfo;
        newAccountInfo.supportedAAVersion = AccountAbstractionVersion.None;
        // Accounts have sequential nonces by default.
        newAccountInfo.nonceOrdering = AccountNonceOrdering.Sequential;
        _storeAccountInfo(_deployment.newAddress, newAccountInfo);

        if (_deployment.callConstructor) {
            _constructContract(_sender, _deployment.newAddress, _deployment.input, false); //<----set codehash to Constructed in _constructContract()
        }

        //<-------but if don't need callConstructor, codehash will still  Constructing

        emit ContractDeployed(_sender, _deployment.bytecodeHash, _deployment.newAddress);
    }

This method first sets codeHash to Constructing If callConstructor is required, _constructContract () will be called, and codehash will be set to Constructed in the _constructContract() But do nothing if don't need callConstructor, so codehash will still Constructing This will cause the code corresponding to the address to remain invalid.

Tools Used

Recommended Mitigation Steps

    function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {
        _ensureBytecodeIsKnown(_deployment.bytecodeHash);
        _storeConstructingByteCodeHashOnAddress(_deployment.newAddress, _deployment.bytecodeHash);

        AccountInfo memory newAccountInfo;
        newAccountInfo.supportedAAVersion = AccountAbstractionVersion.None;
        // Accounts have sequential nonces by default.
        newAccountInfo.nonceOrdering = AccountNonceOrdering.Sequential;
        _storeAccountInfo(_deployment.newAddress, newAccountInfo);

        if (_deployment.callConstructor) {
            _constructContract(_sender, _deployment.newAddress, _deployment.input, false);
-       }            
+       }else{
+           ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.markAccountCodeHashAsConstructed(_newAddress);
+       }

        emit ContractDeployed(_sender, _deployment.bytecodeHash, _deployment.newAddress);
    }

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L223-L228

Vulnerability details

Impact

fallback lack payable,will lead to differences from the mainnet, and many existing protocols may not work

Proof of Concept

DefaultAccount Defined as follows:

DefaultAccount

The implementation of the default account abstraction. This is the code that is used by default for all addresses that are not in kernel space and have no contract deployed on them. This address:

Contains the minimal implementation of our account abstraction protocol. Note that it supports the built-in paymaster flows.
When anyone (except bootloader) calls/delegate calls it, it behaves in the same way as a call to an EOA, i.e. it always returns success = 1, returndatasize = 0 for calls from anyone except for the bootloader.

If there is no code for the address, the DefaultAccount #fallback method will be executed, which is compatible with the behavior of the mainnet

But At present, fallback is not payable The code is as follows

contract DefaultAccount is IAccount {
    using TransactionHelper for *;
..
    fallback() external { //<--------without payable
        // fallback of default account shouldn't be called by bootloader under no circumstances
        assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS);

        // If the contract is called directly, behave like an EOA
    }

    receive() external payable {
        // If the contract is called directly, behave like an EOA
    }

which will lead to differences from the mainnet.

For example, the mainnet execution of the method with value will return true, and the corresponding value will be transfer but DefaultAccount.sol will return false

It is quite common for call () to with value. If it is not compatible, many existing protocols may not work

Mainnet code example, executing 0x0 call with value can be successful:

  function test() external {
    vm.deal(address(this),1000);
    console.log("before value:",address(0x0).balance);
    (bool result,bytes memory datas) = address(0x0).call{value:10}("abc");
    console.log("call result:",result);
    console.log("after value:",address(0x0).balance);
  }

$ forge test -vvv

[PASS] test() (gas: 42361)
Logs:
  before value: 0
  call result: true
  after value: 10

Simulate DefaultAccount #fallback without payable, it will fail:

    contract DefaultAccount {
        fallback() external {     
        }
        receive() external payable {      
        }
    }

    function test() external {
        DefaultAccount defaultAccount = new DefaultAccount();
        vm.deal(address(this),1000);
        console.log("before value:",address(defaultAccount).balance);
        (bool result,bytes memory datas) = address(defaultAccount).call{value:10}("abc");
        console.log("call result:",result);
        console.log("after value:",address(defaultAccount).balance);
    }

$ forge test -vvv

[PASS] test() (gas: 62533)
Logs:
  before value: 0
  call result: false
  after value: 0

Tools Used

Recommended Mitigation Steps

- function test() external {
+ function test() external payable {
    vm.deal(address(this),1000);
    console.log("before value:",address(0x0).balance);
    (bool result,bytes memory datas) = address(0x0).call{value:10}("abc");
    console.log("call result:",result);
    console.log("after value:",address(0x0).balance);
  }