No Allowlist For Bridgeable ERC-20 Tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L111-L132

Vulnerability details

Vulnerability Details

We noticed that the deposit function of the L1ERC20Bridge contract (code snippet 1) permits a user to bridge any ERC-20 tokens (including deflationary and rebase tokens) from the L1 to the L2 network.

We considered that permitting non-standard ERC-20 tokens, such as deflationary or rebase tokens, could cause unexpected behaviors introducing potential attacks on the dApps (Decentralized Applications) running on the L2 network.

For instance, if a deflationary token (e.g., SafeMoon) was bridged to the L2 network, that token would be treated like a standard non-deflationary ERC-20 token. In other words, the same deflationary token on L1 and L2 would have different mechanisms.

SNIPPET: 1
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol

111:    function deposit(
112:        address _l2Receiver,
113:        address _l1Token,
114:        uint256 _amount
115:    ) external payable nonReentrant senderCanCallFunction(allowList) returns (bytes32 txHash) {
116:        uint256 amount = _depositFunds(msg.sender, IERC20(_l1Token), _amount);
117:        require(amount > 0, "1T"); // empty deposit amount
118:
119:        bytes memory l2TxCalldata = _getDepositL2Calldata(msg.sender, _l2Receiver, _l1Token, amount);
120:        txHash = zkSyncMailbox.requestL2Transaction{value: msg.value}(
121:            l2Bridge,
122:            0, // L2 msg.value
123:            l2TxCalldata,
124:            DEPOSIT_ERGS_LIMIT,
125:            new bytes[](0)
126:        );
127:
128:        // Save the deposited amount to claim funds on L1 if the deposit failed on L2
129:        depositAmount[msg.sender][_l1Token][txHash] = amount;
130:
131:        emit DepositInitiated(msg.sender, _l2Receiver, _l1Token, amount);
132:    }

Impact

We considered that permitting non-standard ERC-20 tokens, such as deflationary or rebase tokens, could cause unexpected behaviors introducing potential attacks on the dApps (Decentralized Applications) running on the L2 network.

For this reason, we considered this issue high severity.

Proof of Concept

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L111-L132

Tools Used

VSCode (Manual Review)

Recommended Mitigation Steps

We recommend employing an allowlist for bridgeable ERC-20 tokens to enhance the security of all dApps running on the L2 network.

[GalloDaSballo]

Looks off for High Severity.

[miladpiri]

I don’t think that we should support a blacklist/whitelist of supported token. It is reference bridge implementation for the tokens that don’t have complicated token logic. So, invalid issue!

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

For discussion similar to using "rebase and feeOnTransfer" tokens, we could argue that certain tokens may cause self-rekt, hence a QA finding.

Because the issue was sent as High Severity, am closing as overly inflated

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity