Frontrunning of legacy funds transfer may cause permanent DOS of Transfer of Legacy Funds to SharedBridge

[c4-bot-9]

Lines of code

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

Vulnerability details

Detail

During the migration process, the funds from the legacy bridge is expected to be transferred to the shared bridge by calling L1SharedBridge.transferFundsFromLegacy. This function checks if the token to be transferred is ETH or a Non-ETH token, if it is not ETH, the token balance of the sharedBridge is cached as balanceBefore and the entire token balance of the legacy bridge cached as the amount to be transferred. After this check, the amount is then transferred directly to the sharedBridge by an external call to L1ERC20LegacyBridge.tranferTokenToSharedBridge. Afterward, the balance of the sharedBridge is cached, and the difference between balanceAfter and balanceBefore is required to be equal to the amount.

src: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol;

function transferFundsFromLegacy(address _token, address _target, uint256 _targetChainId) external onlyOwner {
        if (_token == ETH_TOKEN_ADDRESS) {
           ....................................
        } else {
            uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
            uint256 amount = IERC20(_token).balanceOf(address(legacyBridge));
            require(amount > 0, "ShB: 0 amount to transfer");
@>          IL1ERC20Bridge(_target).tranferTokenToSharedBridge(_token, amount);
            uint256 balanceAfter = IERC20(_token).balanceOf(address(this));
@>          require(balanceAfter - balanceBefore == amount, "ShB: wrong amount transferred");
           ........................
    }

Impact

The transfer of legacy funds from L1ERC20LegacyBridge to L1SharedBridge during the migration process can be denied potentially permanently by a malicious actor, causing the migration process to fail or not complete as expected.

Proof of Concept

This vulnerability comes in because the external transfer call to transfer this token balance to the shared bridge contract can be front run due to a race condition, since this migration would occur on ETHEREUM, and a malicious user can directly deposit tokens into the shared bridge before this transfer is completed. When the balanceAfter is called and cached, this balance would be different from what is expected, causing the require check that the difference between balanceAfter and balanceBefore to fail. Consider the following scenario

1. During migration process, L1SharedBridge.transferFundsFromLegacy is called on a token by the owner contract, within the function, after balanceBefore is cached into memory.
2. When tranferTokenToSharedBridge external call is made, a malicious user makes a direct transfer of the tokens to the L1SharedBridge contract, causing the balance of the contract to be changed. When balanceAfter is cached and the difference between balanceAfter and balanceBefore is checked, it'll return a different value than the amount transferred, resulting in a revert. This user can perform this action as many times as necessary to DOS this process until a logic change is made.

   Tools Used

   Manual Review

Recommended Mitigation

1. Implement a pull withdrawal method in L1SharedBridge where instead of a direct transfer from L1ERC20LegacyBridge, the shared bridge contract is approved to spend the entire balance of the legacy bridge and the L1SharedBridge calls this1. function after migration. The below gives a rough idea of how this should be implemented.

 function withdrawFromLegacyBridge(address _token, address _target, address _chainId) external {
        uint256 allowance = IERC20(_token).allowance(address(this), address(L1ERC20LegacyBridge);
        require(allowance > 0, "No allowance granted");

        bool success = IERC20(_token).safeTransferFrom(address(L1ERC20LegacyBridge), address(this), allowance);
        require(success, "Transfer failed");
    }
}

This would prevent any potential race conditions and DOS and allow the migration process to complete without problems.

Assessed type

DoS

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

EVM is a synchronus machine. This cannot happen.

[alex-ppg]

The Warden attempts to pinpoint a front-running attack vector in relation to deposits, however, the code segments mentioned are not vulnerable to such a flaw due to each statement being synchronously executed.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid