Unsafe safeTransfer function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync//blob/main/contracts/openzeppelin/utils/Address.sol#L266 https://github.com/code-423n4/2023-03-zksync//blob/main/contracts/openzeppelin/token/ERC20/utils/SafeERC20.sol#L22

Vulnerability details

Impact

The safeTransfer function of the SafeERC20.sol contract check that the target is actually a contract before calling it, this is to avoid calls to address with no code that will always return succes = true. For exemple if you use token.safeTransfer() and token = address(0),transfer will fail and the transaction will revert thanks to the isContract check.

The isContract use address.code.length > 0 to check if its a contract or not.

zkSync have the concept of Empty contracts, defined like this :

"Some of the contracts are relied upon to have EOA-like behavior, i.e. they can be always called and get the success value in return. An example of such an address is a 0 address. We also require the bootloader to be callable so that the users could transfer ETH to it.

For these contracts, we insert the EmptyContract code upon genesis. It is basically a noop code, which does nothing and returns success=1."

So in zkSync era, the safeTransfer and safeTransferFrom function will work with address that actually contains the EmptyContract code,notably the 0 address.This can lead to unintended behavior for protocols that trust the SafeERC20 contract.

Proof of Concept

As a proof of Concept, i will give an exemple of a known protocol that can get exploited with the 0 address returning true for the isContract check.

The celer bridge : https://polygonscan.com/address/0x88dcdc47d2f83a99cf0000fdf667a468bb958a78#code

There is two bridge function, one to bridge ERC20 and on to bridge Native currency but they use the same event, if you use the 0 address on the sendERC20 function, the transaction will fail. But on zkSync era the transaction will go trough since the 0 address does contain code and you will get native token on the other side of the bridge since the 0 address represents native token.

In the past, qubit finance got exploited because they didnt use the safeERC20 contract and the attacker stole 80 millions dollar with the exact method described above.

https://www.halborn.com/blog/post/explained-the-qubit-hack-january-2022

Their might be other ways to exploit this, but basically every bridge using the same events to bridge ERC20 tokens and Native currency could be exploited because of this.

Tools Used

None

Recommended Mitigation Steps

Change the isContract function of the SafeERC20 contract to block calls to those empty contract address, or at least to the 0 address.

[GalloDaSballo]

Would have benefited by a POC, flagging out of caution

[miladpiri]

The EmptyContract is deployed to some specific addresses like address(0). At the same time, extcodesize(0) will return zero value, according to the implementation of the AccountCodeStorage contract.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

Siding with the sponsor per this piece of code

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid