Barichek - Incorrect implementation of the _isCorrectTokenPair function

[github-actions]

Barichek

medium

Incorrect implementation of the _isCorrectTokenPair function

Summary

The implementation of the _isCorrectTokenPair function depends on the presence of the l1Token function in the _mintableToken implementation. However, if the new IOptimismMintableERC20 interface is used instead of the legacy ILegacyMintableERC20, this function is not available, causing problems in the relationship between the token and the bridge.

Also, in the _isCorrectTokenPair there is used staticcall when calling the l1Token function, but the mentioned interfaces do not have similar view statuses. This also can lead to insolvency of the token <-> bridge relations.

Vulnerability Detail

The _isOptimismMintableERC20 and _isCorrectTokenPair functions in the StandardBridge contract have such implementation:

/**
 * @notice Checks if a given address is an OptimismMintableERC20. Not perfect, but good enough.
 *         Just the way we like it.
 *
 * @param _token Address of the token to check.
 *
 * @return True if the token is an OptimismMintableERC20.
 */
function _isOptimismMintableERC20(address _token) internal view returns (bool) {
    return
        ERC165Checker.supportsInterface(_token, type(ILegacyMintableERC20).interfaceId) ||
        ERC165Checker.supportsInterface(_token, type(IOptimismMintableERC20).interfaceId);
}

/**
 * @notice Checks if the "other token" is the correct pair token for the OptimismMintableERC20.
 *
 * @param _mintableToken OptimismMintableERC20 to check against.
 * @param _otherToken    Pair token to check.
 *
 * @return True if the other token is the correct pair token for the OptimismMintableERC20.
 */
function _isCorrectTokenPair(address _mintableToken, address _otherToken)
    internal
    view
    returns (bool)
{
    return _otherToken == OptimismMintableERC20(_mintableToken).l1Token();
}

They are used in the standard bridge when some ERC-20 token bridge transactions are initiated or finalized. The _isOptimismMintableERC20 function is used to determine whether the token implements an interface that is compatible with the required one (please note, that the actual one interface is IOptimismMintableERC20, and the ILegacyMintableERC20 interface is left here to maintain the backward compatibility with previously deployed tokens). These interfaces are:

/**
 * @title IOptimismMintableERC20
 * @notice This interface is available on the OptimismMintableERC20 contract. We declare it as a
 *         separate interface so that it can be used in custom implementations of
 *         OptimismMintableERC20.
 */
interface IOptimismMintableERC20 {
    function remoteToken() external returns (address);

    function bridge() external returns (address);

    function mint(address _to, uint256 _amount) external;

    function burn(address _from, uint256 _amount) external;
}

/**
 * @custom:legacy
 * @title ILegacyMintableERC20
 * @notice This interface was available on the legacy L2StandardERC20 contract. It remains available
 *         on the OptimismMintableERC20 contract for backwards compatibility.
 */
interface ILegacyMintableERC20 is IERC165 {
    function l1Token() external returns (address);

    function mint(address _to, uint256 _amount) external;

    function burn(address _from, uint256 _amount) external;
}

As mentioned above, only the IOptimismMintableERC20 interface is required to be a standard bridge-compatible token (and the ILegacyMintableERC20 is an optional one). The "notice" comment above the declaration of the IOptimismMintableERC20 interface confirms it. But the _isCorrectTokenPair is based on the fact that the token which was considered in the _isOptimismMintableERC20 function as an appropriate will implement the l1Token function -- which is not true as such function is not a part of the IOptimismMintableERC20 interface (and only part of the ILegacyMintableERC20 interface). This leads to the impossibility of using tokens that implement the declared "expected" interface when working with a standard bridge.

The same applies to the type of the call inside of the _isCorrectTokenPair function -- currently, it is staticcall (according to the view declaration of the l1Token function in the OptimismMintableERC20 contract), but in ILegacyMintableERC20 and IOptimismMintableERC20 interfaces mentioned getter methods are not view, which means that in case they are implemented in a way that they are changing state such call will also fail.

Impact

Any optimism mintable ERC-20 token that does not implement the l1Token function, but instead uses a custom implementation that is compatible with the IOptimismMintableERC20 interface, will be impossible to bridge through the standard bridge. Even though the proposed default implementation of OptimismMintableERC20 has the mentioned legacy function, the contract does not work as expected by the users.

In some cases, such as if a token contract upgrades its implementation to remove mentioned legacy function (which logically can be safely removed if the IOptimismMintableERC20 interface is implemented correctly), it will become unusable in a standard bridge (there will be no chance to bridge token in any way), leading to great reputational and financial losses.

Also, for example, we can consider the case when the project is launched on optimism, its token counterparts are created on L1 and L2 with some custom minting conditions not only for the standard bridge (and at the same time there is a critical assumption than the token must still be working with the L2 side of the standard bridge) -- in this case, the financial assumptions of the project may be violated, which can also lead to reputational and financial losses for the project and whole ecosystem.

Please note that this is not just a discrepancy between documentation and implementation, as in this particular case there is a bug in the code implementation (regarding the correct implementation and logic of the function), not in the documentation.

Code Snippet

• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L446
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L460
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/IOptimismMintableERC20.sol#L12
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/IOptimismMintableERC20.sol#L28
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol#L119

Tool used

Manual Review

Recommendation

Use a separate logic in the _isCorrectTokenPair function for all mentioned interfaces of the _isOptimismMintableERC20 function.

As an example, you can do the following (which also can be optimized by storing in memory the supported interface type found during the _isOptimismMintableERC20 function call):

/**
 * @notice Checks if a given address is an OptimismMintableERC20. Not perfect, but good enough.
 *         Just the way we like it.
 *
 * @param _token Address of the token to check.
 *
 * @return True if the token is an OptimismMintableERC20.
 */
function _isCorrectTokenPair(address _mintableToken, address _otherToken)
    internal
    view
    returns (bool)
{
    if (ERC165Checker.supportsInterface(_mintableToken, type(ILegacyMintableERC20).interfaceId)) {
        return _otherToken == ILegacyMintableERC20(_mintableToken).l1Token();
    } else {
        return _otherToken == IOptimismMintableERC20(_mintableToken).remoteToken();
    }
}

Another similar way to do this is to not divide the logic into two functions _isOptimismMintableERC20 and _isCorrectTokenPair, but instead use the one that returns the bool indicator if the token is optimism mintable ERC-20 and the address of the counterpart of it in case it is so.

Also, add to the declaration of the corresponding function of the IOptimismMintableERC20 interface view keyword.

Duplicate of #220

[rcstanciu]

Comment from Optimism

---

Severity: Medium

Reason: This is a bug in our code that 'locks-in' the legacy interface, and would freeze funds in an non-legacy

mintable token.

Action: Fix so that it can handle either legacy or new token interface.