code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In function withdraw

 function withdraw(address _l1Receiver) external payable override {
    uint256 amount = msg.value;

    // Silent burning of the ether
    unchecked {

        balance[address(this)] -= amount;
        totalSupply -= amount;
    }

they are using unchecked to decrease balances. The issue here is that they are not checking that balance[address(this)] -= amount > 0, so if the msg.sender is sending more ether than what it is holded in the contract, the balance will underflow for the mapping: balance[address(this)] -= amount;

Proof of Concept

Case: balance[address(this)] -= amount = 100 we are sending 500

 interface Underflow{
 function withdraw(address _l1Receiver) external payable override ;
}

 contract AttackUnder{

function Pay()external{
    Underflow(0xdD870fA1b7C4700F2BD7f44238821C26f7392148).withdraw{value:500}(address(this));
 }
}

After: balance[address(this)] -= amount = 2 **256 - 400;

Tools Used

Manual

Recommended Mitigation Steps

Check that balance[address(this)] -= amount > 0 before burning

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #110

miladpiri commented 1 year ago

The balance of L2EthToken contract is increased with the msg.value provided, so it is fine to decrease it. In other words, when msg.value is transfered to do the withdraw, the funds are transfered to the balance of the L2ETHToken.

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed