incorrect Handling of Non-Standard ERC20 Token Transfers

[c4-bot-5]

Lines of code

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

Vulnerability details

description

there is a vulenrbaility arise in the function deposit in this line require(amount == _amount, "3T");,this line here where amount is the actual amount received by the shared bridge contract, and _amount is the amount the user intended to deposit. For tokens deducting fees on transfer, amount will be less than _amount, causing the transaction to revert, this This behavior can lead to failed deposit attempts for users attempting to bridge fee-on-transfer tokens, and resulting in a poor user experience and potential loss of funds due to transaction fees. Additionally, it restricts the usability of the bridge for a subset of tokens, reducing the contract's overall utility.

• users attempting to deposit non-standard tokens may face transaction failures without a clear understanding of the cause.

• The bridge's inability to handle non-standard tokens limits its utility and flexibility, potentially excluding a range of tokens from being bridged.

  require(_amount != 0, "0T"); // empty deposit
      uint256 amount = _depositFundsToSharedBridge(msg.sender, IERC20(_l1Token), _amount);
      require(amount == _amount, "3T"); // The token has non-standard transfer logic
  
      l2TxHash = sharedBridge.depositLegacyErc20Bridge{value: msg.value}(
          msg.sender,
          _l2Receiver,
          _l1Token,
          _amount,
          _l2TxGasLimit,
          _l2TxGasPerPubdataByte,
          _refundRecipient
      );
      depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
      emit DepositInitiated(l2TxHash, msg.sender, _l2Receiver, _l1Token, _amount);
  }

  the vulnerability is from that the contract fails to account for discrepancies between the intended deposit amount and the amount actually transferred when dealing with non-standard ERC20 tokens.

  impact

this discrepancy could lead to failed transactions or incorrect bridging amounts being recorded, especially in tokens deducting fees on transfers.

Proof of Concept

i fuzz with scenario that simulate token transfers with a 1% fee deduction on each transactionand and the contract's logic was expected to transfer a specified amount to the shared bridge, and the fuzz test checked if the actual transferred amount matched the specified amount and get some result that the test not pass because the actual transferred amount (436.59 tokens) was less than the specified amount due to the 1% fee deduction, revealing that the contract does not handle non-standard token logic as expected.: Specified Deposit Amount: 441 tokens Simulated Transferred Amount (after 1% fee deduction): 436.59 tokens Expected by Contract (for a successful deposit): 441 tokens import random

# Simulate the non-standard ERC20 token behavior
def simulate_token_transfer(amount):
    """
    Simulates a token transfer with a 1% fee deduction.
    """
    fee_deduction = amount * 0.01  # 1% fee
    transferred_amount = amount - fee_deduction
    return transferred_amount

# Mock the _depositFundsToSharedBridge function
def mock_deposit_funds_to_shared_bridge(deposit_amount):
    """
    Simulates depositing funds to the shared bridge, considering the token's transfer behavior.
    """
    balance_before = 1000  # Assume initial balance
    transferred_amount = simulate_token_transfer(deposit_amount)
    balance_after = balance_before + transferred_amount
    return balance_after - balance_before

# Fuzz test with random amounts
test_results = []
for _ in range(10):  # Run 10 iterations
    deposit_amount = random.randint(1, 1000)  # Random deposit amount between 1 and 1000
    actual_transferred_amount = mock_deposit_funds_to_shared_bridge(deposit_amount)
    test_result = {
        "Deposit Amount": deposit_amount,
        "Actual Transferred": actual_transferred_amount,
        "Expected": deposit_amount,
        "Test Passed": actual_transferred_amount == deposit_amount
    }
    test_results.append(test_result)

test_results

so when a user attempts to deposit tokens into the bridge, expecting a 1:1 transfer, the actual amount that gets bridged is less than expected due to the fee deduction by the token. However, the contract checks for an exact match between the specified deposit amount and the transferred amount, leading to potential failures or incorrect bridging amounts being recorded.

Tools Used

manual review

Recommended Mitigation Steps

• removing the strict equality check and instead validating that the received amount is within an acceptable range of the intended deposit amount (_amount).
• and should handle the behaviors of non-standard tokens,as fee-on-transfer tokens, by calculating the expected fees and adjusting the deposit logic accordingly.

Assessed type

Other

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

The inclusion of tokens is currently permissioned and we do not support any non-standard implementation of ERC20s. This is an invalid issue for us.

[alex-ppg]

The Warden specifies that the L1ERC20Bridge is incompatible with fee-on-transfer tokens and in general, any token that may transact less or more than the specified amount to the end-user. The impact of this is that the transaction will fail, and based on the Sponsor's clarification that they do not intend to support those tokens which the present code explicitly does, I consider this exhibit out-of-scope / better suited as part of an Analysis report.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope