Closed github-actions[bot] closed 1 year ago
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.
Barichek
medium
Incorrect implementation of the
_isCorrectTokenPair
functionSummary
The implementation of the
_isCorrectTokenPair
function depends on the presence of thel1Token
function in the_mintableToken
implementation. However, if the newIOptimismMintableERC20
interface is used instead of the legacyILegacyMintableERC20
, this function is not available, causing problems in the relationship between the token and the bridge.Also, in the
_isCorrectTokenPair
there is usedstaticcall
when calling thel1Token
function, but the mentioned interfaces do not have similarview
statuses. This also can lead to insolvency of the token <-> bridge relations.Vulnerability Detail
The
_isOptimismMintableERC20
and_isCorrectTokenPair
functions in theStandardBridge
contract have such implementation: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 isIOptimismMintableERC20
, and theILegacyMintableERC20
interface is left here to maintain the backward compatibility with previously deployed tokens). These interfaces are:As mentioned above, only the
IOptimismMintableERC20
interface is required to be a standard bridge-compatible token (and theILegacyMintableERC20
is an optional one). The "notice" comment above the declaration of theIOptimismMintableERC20
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 thel1Token
function -- which is not true as such function is not a part of theIOptimismMintableERC20
interface (and only part of theILegacyMintableERC20
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 theview
declaration of thel1Token
function in theOptimismMintableERC20
contract), but inILegacyMintableERC20
andIOptimismMintableERC20
interfaces mentioned getter methods are notview
, 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 theIOptimismMintableERC20
interface, will be impossible to bridge through the standard bridge. Even though the proposed default implementation ofOptimismMintableERC20
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
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):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
interfaceview
keyword.Duplicate of #220