ERC20Vault and ERC721Vault do not work with all valid ERC20 and ERC721 compliant tokens

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L369 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC721Vault.sol#L206

Vulnerability details

Impact

According to the README here, all vaults are designed to work with all ERC20, ERC721 and ERC1155 tokens, respectively.

The issue is with the ERC20Vault and ERC721Vault contracts which expect canonical tokens to implement the functions name(), symbol() (in case of both ERC20 and ERC721) and decimals() (in case of ERC20 only).

According to the EIP-20 specification, functions name(), symbol() and decimals() are OPTIONAL and other contracts (in this case the vaults) MUST NOT expect these values to be present.

OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.

According to the EIP-721 specification, the metadata extension, which includes functions name() and symbol(), is optional.

The metadata extension is OPTIONAL for ERC-721 smart contracts

Since both the ERC20Vault and ERC721Vault expect these functions to be present by calling them here and here, the calls would revert. This disallows valid ERC-20 and ERC=721 compliant canonical tokens from being bridged.

The issue does not exist in the ERC1155Vualt since it considers for this situation here by implementing a try-catch block.

Proof of Concept

When the function sendToken() in the ERC20Vault/ERC721Vault is called by the user, it internally calls function _handleMessage(). If the _token parameter is a canonical token, we enter the else block on Line 376 for ERC20Vault and Line 204 for ERC721Vault.

In the else block, we can see that the functions name(), symbol() and decimals() are directly being called on the canonical token. Since these are optional, the function will revert and prevent users from bridging valid ERC20 and ERC721 compliant tokens.

Function _handleMessage() in ERC20Vault:

File: ERC20Vault.sol
358:     function _handleMessage(
359:         address _user,
360:         address _token,
361:         address _to,
362:         uint256 _amount
363:     )
364:         private
365:         returns (
366:             bytes memory msgData_,
367:             CanonicalERC20 memory ctoken_,
368:             uint256 balanceChange_
369:         )
370:     {
371:         // If it's a bridged token
372:         if (bridgedToCanonical[_token].addr != address(0)) {
373:             ctoken_ = bridgedToCanonical[_token];
374:             IBridgedERC20(_token).burn(msg.sender, _amount); 
375:             balanceChange_ = _amount;
376:         } else {
377:             // If it's a canonical token
378:             IERC20Metadata meta = IERC20Metadata(_token);
379:             ctoken_ = CanonicalERC20({
380:                 chainId: uint64(block.chainid),
381:                 addr: _token,
382:                 decimals: meta.decimals(),
383:                 symbol: meta.symbol(),
384:                 name: meta.name()
385:             });

Function _handleMessage() in ERC721Vault:

File: ERC721Vault.sol
190:     function _handleMessage(
191:         address _user,
192:         BridgeTransferOp memory _op
193:     )
194:         private
195:         returns (bytes memory msgData_, CanonicalNFT memory ctoken_)
196:     {
197:         unchecked {
198:             if (bridgedToCanonical[_op.token].addr != address(0)) {
199:                 ctoken_ = bridgedToCanonical[_op.token];
200:                 for (uint256 i; i < _op.tokenIds.length; ++i) {
201:                     
202:                     BridgedERC721(_op.token).burn(_user, _op.tokenIds[i]);
203:                 }
204:             } else {
205:                 ERC721Upgradeable t = ERC721Upgradeable(_op.token);
206: 
207:                 ctoken_ = CanonicalNFT({
208:                     chainId: uint64(block.chainid),
209:                     addr: _op.token,
210:                     symbol: t.symbol(), 
211:                     name: t.name()
212:                 });

Unlike the two vaults above, the ERC1155Vault considers this case by implementing a try-catch block on Line 286 for the functions name() and symbol().

Function _handleMessage() in ERC1155Vault:

File: ERC1155Vault.sol
260:     function _handleMessage(
261:         address _user,
262:         BridgeTransferOp memory _op
263:     ) private returns (bytes memory msgData_, CanonicalNFT memory ctoken_) {
264:         unchecked {
265:             // is a btoken, meaning, it does not live on this chain
266:             if (bridgedToCanonical[_op.token].addr != address(0)) {
267:                 ctoken_ = bridgedToCanonical[_op.token];
268:                 for (uint256 i; i < _op.tokenIds.length; ++i) {
269:                     
270:                     BridgedERC1155(_op.token).burn(
271:                         _user,
272:                         _op.tokenIds[i],
273:                         _op.amounts[i]
274:                     );
275:                 }
276:             } else {
277:                 // is a ctoken token, meaning, it lives on this chain
278:                 ctoken_ = CanonicalNFT({
279:                     chainId: uint64(block.chainid),
280:                     addr: _op.token,
281:                     symbol: "",
282:                     name: ""
283:                 });
284:                 IERC1155NameAndSymbol t = IERC1155NameAndSymbol(_op.token);
285:              
286:                 try t.name() returns (string memory _name) {
287:                     ctoken_.name = _name;
288:                 } catch {}
289:                 try t.symbol() returns (string memory _symbol) {
290:                     ctoken_.symbol = _symbol;
291:                 } catch {}
292:                 for (uint256 i; i < _op.tokenIds.length; ++i) {
293:                     IERC1155(_op.token).safeTransferFrom({
294:                         from: msg.sender,
295:                         to: address(this),
296:                         id: _op.tokenIds[i],
297:                         amount: _op.amounts[i],
298:                         data: ""
299:                     });
300:                 }
301:             }
302:         }

Note: For the EIP-20 specification, there is a bit of ambiguity whether the functions themselves are optional or the values for the three functions.

In case the values for the functions name(), symbol() and decimals() are optional, there is still a problem though.

When a BridgedERC20 token is deployed on the destination chain through function _deployBridgedToken() (this happens during bridging a canonical token for the first time from source), the init() function on the BridgedERC20 contract validates the name and symbol inputs here, which expects them to be non-zero as seen here in LibBridgedToken. Due to the possibility of the values being empty as per the specification, the call would revert on the destination chain, causing the same impact of valid ERC20 compliant canonical token being devoid of bridging. The same applies for the ERC721 case.

File: BridgedERC20.sol
52:     function init(
53:         address _owner,
54:         address _addressManager,
55:         address _srcToken,
56:         uint256 _srcChainId,
57:         uint8 _decimals,
58:         string memory _symbol,
59:         string memory _name
60:     ) external initializer {
61:         // Check if provided parameters are valid
62:         LibBridgedToken.validateInputs(_srcToken, _srcChainId, _symbol, _name);
63:         __Essential_init(_owner, _addressManager);
64:         __ERC20_init(_name, _symbol);
65:         __ERC20Snapshot_init();

Once again, we can see that this issue does not exist in the ERC1155Vault contract since it is mitigated by the team by passing placeholder data on Line 53.

File: BridgedERC1155.sol
38:     function init(
39:         address _owner,
40:         address _addressManager,
41:         address _srcToken,
42:         uint256 _srcChainId,
43:         string memory _symbol,
44:         string memory _name
45:     )
46:         external
47:         initializer
48:     {
49:         // Check if provided parameters are valid.
50:         // The symbol and the name can be empty for ERC1155 tokens so we use some placeholder data
51:         // for them instead.
52:      
53:         LibBridgedToken.validateInputs(_srcToken, _srcChainId, "foo", "foo");
54:         __Essential_init(_owner, _addressManager);
55:         __ERC1155_init(LibBridgedToken.buildURI(_srcToken, _srcChainId));
56: 
57:         srcToken = _srcToken;
58:         srcChainId = _srcChainId;
59:         __symbol = _symbol;
60:         __name = _name;
61:     }

Tools Used

Manual Review

Recommended Mitigation Steps

Similar to the ERC1155Vault, consider implementing a try-catch block for name(), symbol() and decimals() in ERC20Vault and ERC721Vault.

Note that the BridgedERC20 and BridgedERC721 token contracts would need to be updated as well since they currently validate the name and symbol inputs here and here to be non-zero values. Since according to the EIP, these values can be empty, the calls on destination would always revert. This issue does not exist in BridgedERC1155 since it uses placeholder data here to avoid reverting when a new bridge token is being deployed/created for the canonical token through the vault here.

Assessed type

ERC20

[c4-pre-sort]

minhquanym marked the issue as insufficient quality report

[minhquanym]

OOS in bot report

[c4-judge]

0xean marked the issue as unsatisfactory: Out of scope

[mcgrathcoutinho]

Hi @0xean, I think this issue should technically be valid due to multiple reasons:

1. First, let's understand the scope of tokens the bridge interacts with as per the README here on the first line. This means the all ERC-XXX compliant tokens should be considered.

   Our vaults are designed to work with all ERC20, ERC1155, and ERC721 tokens, respectively.

2. Regarding the low-severity issues covered by the bot report here, we can see that it only covers decimals() in [L-03] and symbol() in [L-10]. It nowhere mentions the name() function (even in it's instances) which is optional for both the ERC20 and ERC721 specifications (as detailed in my report above).

3. Additionally, the bot report does not entail instances and mention another pain point in the execution flow which prevents the bridging i.e. when the bridged token is being deployed on the destination chain, the validateInput() function reverts when the name and symbol values are empty (same case for ERC721).

Overall, I think the bot report is insufficient in detailing the issue and instances where the issues in the execution flow could've arised.

Thank you for your time!

[0xean]

will upgrade to QA.

[c4-judge]

0xean removed the grade

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a

[mcgrathcoutinho]

Hi @0xean, I believe this issue should a valid Medium-severity.

1. The README clearly mentions that the vaults are designed to work with all ERC20 and ERC721 compliant tokens. In our case, this is not true.

   Our vaults are designed to work with all ERC20, ERC1155, and ERC721 tokens, respectively.

2. As pointed in the above comment, the bot report only mentions symbol() and decimals() but not name() anywhere. Even after considering the bot report for symbol() and decimals(), the destination chain would still revert due to the validateInput() function here checking empty values here.

In short, the bot report does not include function name() anywhere and an additional issue is being overlooked in the execution flow on the destination chain.

Overall, since the intended functionality of the protocol is impacted (i.e. valid ERC20 and ERC721 tokens are devoid of being bridged), I think this should be upgraded to Medium.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[mcgrathcoutinho]

Tagging @adaki2004 @dantaik to gather their view.

I think this should clearly classify as a Medium-severity issue based on the C4 docs Med definition (intended functionality of the protocol defined by sponsors in the README is not available) and the two points mentioned above.

[0xean]

this is finalized.

[adaki2004]

Tagging @adaki2004 @dantaik to gather their view.

I think this should clearly classify as a Medium-severity issue based on the C4 docs Med definition (intended functionality of the protocol defined by sponsors in the README is not available) and the two points mentioned above.

I'd rather say that this description is not fully correct on our side - because there can be contracts which are simply just not possible to work with - for example the aforementioned reasons. Our vaults are designed to work with all ERC20, ERC1155, and ERC721 tokens, respectively. If clearly this can be an issue - it is during initial bridgind so worst case what could happen is the revert during bridging. No assets are at risk, but I'd consider it a QA (wrong commenting), maximum low severity, personally.

[mcgrathcoutinho]

Hi @adaki2004 and @0xean,

I'm sure that the protocol was intended to work with atleast ERC20-compliant and ERC721-compliant tokens. I would've agreed if this was something about rebasing tokens or other weird tokens but the README had clearly defined this during the contest, which could also be seen in how ERC1155Vault handles the name(), symbol() and decimals() issue here I'm mentioning above in my report.

Regarding the severity, I think it classifies as Medium since the intended functionality defined is not available. Here is the C4 docs:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[adaki2004]

How does it complies with this below ? function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I dont see functions impacted, but only transactions rejected, we can argue about semantics that functions vs. transactions but dont think it makes sense.

I agree and will not deny that e.g.: name is option in ERC20() but can you name any (valuable/semi-valuable) coins that are lacking these functionalities ? Also in case they are lacking the name / smybol -> What would be the name on our L2 ? Shall we make up a random name/symbol for them ?

If the probability to happen is low, impact is (max) medium (impact is: coin is not able to be bridged, but no loss of funds, simply no support.) -> This is a abs. max a low. Sorry, usually i'm flexible and happy to judge in favor of warden, but the best i would offer here is a low.

[0xean]

@mcgrathcoutinho - this is finalized, if you decide to continue arguing I will downgrade this to C

[mcgrathcoutinho]

My intention was not to argue, sorry if I sounded like that.

Though a low-severity issue, I hope the sponsor found this issue to be valuable.

Thank you for your responses. Will agree to the current ruling by the judge and the sponsor comments above.