Zellic - Audit Reports

Category: Business Logic

Protocol does not account for fee-on-transfer tokens

Low Severity

Low Impact

Low Likelihood

Description

The _initiateERC20Deposit function does not account for tokens that charge fees on transfer.

There is an expectation that the _amount of tokens deposited to the project contract when calling depositERC20To or depositERC20 will be equal to the amount of tokens deposited, and hence the mapping deposits is updated by adding the same _amount. However, there are ERC-20s that do, or may in the future, charge fees on transfer that will violate this expectation and affect the contract's accounting in the deposits mapping.

Below is the function _initiateERC20Deposit from the L1StandardBridge contract (some part of the function is replaced by // [removed code] to only show the relevant code):

function _initiateERC20Deposit(
    address _l1Token,
    address _l2Token,
    address _from,
    address _to,
    uint256 _amount,
    uint32 _l2Gas,
    bytes calldata _data
) internal {
    // When a deposit is initiated on L1, the L1 Bridge transfers the funds to itself for future
    // withdrawals. The use of safeTransferFrom enables support of "broken tokens" which do not
    // return a boolean value.
    // slither-disable-next-line reentrancy-events, reentrancy-benign
    IERC20(_l1Token).safeTransferFrom(_from, address(this), _amount);

    // [removed code]

    // slither-disable-next-line reentrancy-benign
    deposits[_l1Token][_l2Token] = deposits[_l1Token][_l2Token] + _amount;

    // slither-disable-next-line reentrancy-events
    emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data);
}

Impact

The deposits mapping will overestimate the amount of fee-on-transfer tokens in the contract.

Recommendations

Consider implementing a require check that compares the contract's balance before and after a token transfer to ensure that the expected amount of tokens are transferred.

    function _initiateERC20Deposit(
        address _l1Token,
        address _l2Token,
        address _from,
        address _to,
        uint256 _amount,
        uint32 _l2Gas,
        bytes calldata _data
    ) internal {
        // When a deposit is initiated on L1, the L1 Bridge transfers the funds to itself for future
        // withdrawals. The use of safeTransferFrom enables support of "broken tokens" which do not
        // return a boolean value.
        // slither-disable-next-line reentrancy-events, reentrancy-benign
+       uint256 expectedTransferBalance =
        IERC20(_l1Token).balanceOf(address(this)) + _amount;
        IERC20(_l1Token).safeTransferFrom(_from, address(this), _amount);
+       uint256 postTransferBalance = 
        IERC20(_l1Token).balanceOf(address(this));
+       require(expectedTransferBalance == 
        postTransferBalance, "Fee on transfer tokens not supported");

        // [removed code]

        // slither-disable-next-line reentrancy-benign
        deposits[_l1Token][_l2Token] = deposits[_l1Token][_l2Token] + _amount;

        // slither-disable-next-line reentrancy-events
        emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data);
    }

Remediation

This issue has been acknowledged by Mantle Network, and a fix was implemented in commit 305b5cab↗.