`DefaultAccount` does not accept transfers with calldata

[code423n4]

Lines of code

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

Vulnerability details

Impact

The fallback function in DefaultAccount is defined as fallback() external {...}, but it must be fallback() external payable {...}. This bug causes transfers with calldata to default accounts always to be reverted. On the other hand, they are not be reverted in Ethereum. Since DefaultAccount is required to behave like EOA, this fact is inconsistent with the specification.

In addition, in the ignoreNonBootloader and ignoreInDelegateCall modifiers, there is the following statement, but as mentioned above, DefaultAccount implements an empty fallback instead of an empty payable fallback, so it can be distinguished.

If all functions will use this modifier AND the contract will implement an empty payable fallback() then the contract will be indistinguishable from the EOA when called.

This bug makes it impossible to transfer Ether to any default account from any contract that implements transfers with calldata. In particular, if Ether deposited in a contract can only be withdrawn via a transfer with calldata, the assets will be permanently frozen.

For example, calling the returnEther function of the following contract has no risk in Ethereum. The Ether is simply transferred to Foo and returned. However, in zkSync Era, the above bug causes the Ether to not be returned, and it remains in Foo and is lost.

contract Foo {
    function returnEther() public payable {
        payable(msg.sender).call{value: msg.value}("00");
    }
}

Transfers with calldata are general, as seen in 2023-03-zksync/blob/main/contracts/openzeppelin/utils/Address.sol#L159 (this functionCallWithValue checks for success, so the asset is not lost and only reverted). Therefore, the possibility of asset loss in various protocols due to this vulnerability is very high.

Proof of Concept

The code causing this vulnerability: https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L223

The test below is a transfer with calldata to a default account address (0x100000); for Ethereum, this succeeds, but for zkSync Era, it fails. This is dangerous.

//! { "cases": [ {
//!     "name": "test",
//!     "inputs": [
//!         {
//!             "method": "test",
//!             "calldata": [
//!             ],
//!             "value": "1 ETH"
//!         }
//!     ],
//!     "expected": [
//!     ]
//! } ] }

// SPDX-License-Identifier: UNLICENSED 

pragma solidity >=0.4.16;

contract Test {
    function test() external payable {
        address defaultAccountAddress = address(0x100000);
        (bool success, ) = payable(defaultAccountAddress).call{value: msg.value}("00");
        require(success); // failed
    }
}

The above vulnerability can cause users' assets to be lost in a variety of scenarios. For example, the following test shows that the user's assets are locked to the Foo contract forever. Since it seems to be not simple to call transfer with calldata from DefaultAccount using era-compiler-tester, SimplifiedDefaultAccountForTest compatible with DefaultAccount is used. The result is the same when DefaultAccount is used.

//! { "cases": [ {
//!     "name": "test",
//!     "inputs": [
//!         {
//!             "method": "test",
//!             "calldata": [
//!             ],
//!             "value": "1 ETH"
//!         }
//!     ],
//!     "expected": [
//!     ]
//! } ] }

// SPDX-License-Identifier: UNLICENSED 

pragma solidity >=0.4.16;

contract Test {
    function test() external payable {
        // A simplified contract is used instead of a DefaultAccount contract, but the result is the same.
        SimplifiedDefaultAccountForTest account = new SimplifiedDefaultAccountForTest{value: 1 ether}();
        Foo foo = new Foo();
        account.callFooReturnEther(address(foo));
        require(address(account).balance == 1 ether); // failed
    }
}

uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000;
address payable constant BOOTLOADER_FORMAL_ADDRESS = payable(address(SYSTEM_CONTRACTS_OFFSET + 0x01));

contract SimplifiedDefaultAccountForTest {
    constructor() payable {} 

    // This process is implemented as a function here, but in reality, it is a transaction by an EOA.
    function callFooReturnEther(address fooAddress) external {
        Foo(fooAddress).returnEther{value: address(this).balance}();
    }

    fallback() external {
        // 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
    } 
}

contract Foo {
    function returnEther() public payable {
        payable(msg.sender).call{value: msg.value}("00");
    }
}

The above test will succeed by changing fallback() external to fallback() external payable.

Tools Used

None

Recommended Mitigation Steps

Change from fallback() external {...} to fallback() external payable {...} in DefaultAccount.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #93

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)