`depositAmount` is not properly updated in `L1ERC20Bridge.deposit()`

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L154

Vulnerability details

Impact

Function L1ERC20Bridge.deposit() initiates a deposit by locking funds on the contract and sending the request of processing an L2 transaction where tokens would be minted. The amount of deposit is being assigned to depositAmount[msg.sender][_l1Token][l2TxHash] mapping. However, calling deposit() does not increase depositAmount[msg.sender][_l1Token][l2TxHash], but overwrites the old value of depositAmount[msg.sender][_l1Token][l2TxHash]. This implies, that when user calls deposit() more than once - he will suffer for the fund loss, because the old value of depositAmount[msg.sender][_l1Token][l2TxHash] will be overwritten by the new value.

Proof of Concept

File: L1ERC20Bridge.sol

depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;

As demonstrated above, depositAmount[msg.sender][_l1Token][l2TxHash] is not increased by _amount, but it is simply overwritten.

1. User calls deposit() with _amount set to 100.
2. User calls deposit() again, with _amount set to 50.
3. depositAmount[msg.sender][_l1Token][l2TxHash] should point to 150 (100 from the first call + 50 from the second call).
4. However, because of the = operator, depositAmount[msg.sender][_l1Token][l2TxHash] actually points to 50 (call from the 2. stage overwrote the _amount set during the 1. stage).
5. User suffers for loss, because the mapping depositAmount[msg.sender][_l1Token][l2TxHash] points that his deposit is only 50, while he deposited 150.

Tools Used

Manual code review

Recommended Mitigation Steps

Change:

depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;

to:

depositAmount[msg.sender][_l1Token][l2TxHash] += _amount;

Assessed type

Context

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

Considered invalid, because l2TxHash is a unique identifier

[alex-ppg]

Based on the rationale laid out in #15 this exhibit is invalid. The l2TxHash is a unique identifier, and thus an increment instead of an overwrite will not resolve the many issues that would arise from an l2TxHash collision.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid