ZanyBonzy - Restriction on bridging polygon tokens not fully effective

[sherlock-admin2]

ZanyBonzy

medium

Restriction on bridging polygon tokens not fully effective

Summary

Polygon MATIC has been upgraded to a new address. The codebase puts a restriction on bridging MATIC tokens but uses the old MATIC address to check for these tokens to prevent bridge transfer. The new address can be used to potentially bypass the bridge check.

Vulnerability Detail

From the readme and description of the BridgeRelay contract, one of the key features is the prevention of direct MATIC bridging through explicit checks.

And to enforce this, the bridgeTransfer contains the check.

    function bridgeTransfer(IERC20 token) external payable {
        // revert if MATIC is attempted
        if (token == MATIC) revert MATICUnbridgeable();
...
    }

And from the declared addresses, the declared MATIC address is the old address

    IERC20 public constant MATIC =
        IERC20(0x7D1AfA7B718fb893dB30A3aBc0Cfc608AaCfeBB0);

This is because MATIC has been upgraded and migrated to a new address, according to their announcement.

Since the restriction is placed on the old address, the new address will still bypass it making it less effective .

Impact

Bridging MATIC tokens against the protocol's restrictions.

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/21920190e0772afa18e7f856a036fea3ef5b9635/telcoin-contracts/contracts/bridge/BridgeRelay.sol#L26-L27

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/21920190e0772afa18e7f856a036fea3ef5b9635/telcoin-contracts/contracts/bridge/BridgeRelay.sol#L46

Tool used

Manual Code Review

Recommendation

Include a check for the new token in the bridgeTransfer function.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

invalid under Sherlock's rules -> future issues

[WangSecurity]

Sorry for the mistake, future issue rule indeed cannot be applied here. Tagging sponsor, @amshirif, for additional info if it’s actually a mistake or was made on purpose

[midori-fuse]

Escalate

Delegating escalation to @ZanyBonzy based on the above two comments. It is correct that the README states one of the key features as:

Prevents direct MATIC bridging through explicit checks.

However I also think that the report hasn't been able to show damage to other users or to the protocol. Combined with the fact that the user has to actually intend to bridge MATIC which is stated to not supported, I think this is a borderline low/med.

Overall I think it's worth requesting a rejudge. This issue is at least correct, so the only thing to consider is how the Sherlock rules and judges view this issue.

[sherlock-admin2]

Escalate

Delegating escalation to @ZanyBonzy based on the above two comments. It is correct that the README states one of the key features as:

Prevents direct MATIC bridging through explicit checks.

However I also think that the report hasn't been able to show damage to other users or to the protocol. Combined with the fact that the user has to actually intend to bridge MATIC which is stated to not supported, I think this is a borderline low/med.

Overall I think it's worth requesting a rejudge. This issue is at least correct, so the only thing to consider is how the Sherlock rules and judges view this issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[amshirif]

@ZanyBonzy I think we talked about this in discord. I think this is a valid issue if POL is now the native currency on Polygon POS. But it appears to me that this is just a token reboot but is not compatible with bridging to Polygon POS. We do not operate on zkEVM. I've attempted to bridge POL but it says it's not a compatible. If you can provide evidence that the new token has replaced MATIC in that capacity I would be happy to consider it valid, but when we spoke I wasnt under the impression that Polygon simply made a new token but did not update the infrastructure around existing protocols to accommodate.

[WangSecurity]

Then it looks as invalid as I understand it, cause MATIC is actually restricted, but POL token isn't, correct?

[Czar102]

It seems so, I don't see why would this issue be valid. What is the loss of funds/impact as @midori-fuse mentioned? Why is it so important to enforce the inability of a MATIC-correlated asset to be not supported?

I'm planning to reject the escalation and leave the issue as is.

[WangSecurity]

I can see that the MATIC shouldn't be bridged. But, as I understand the new address you talk about is indeed POL, not MATIC (yes, it will replace matic, but it's a different token). Secondly, as Czar said, what are the losses? Yes, the code says that MATIC shouldn't be bridged, but I don't see how bridging POL breaks core functionality, rendering the contract useless or causes loss of funds.

If it's a new address of MATIC (not POL), then yeah, might be valid, but if it's actually a different token (that will replace MATIC), then it is low maximum, or even info, cause it doesn't break the functionality or renders contract useless (IMO).

[WangSecurity]

Yeah, they're quite the same, but still it's two tokens, even tho they're quite the same and POL will replace MATIC at some point, but still MATIC specifically is restricted. Also, as was said a couple of times above, it doesn't break core functionality, render contract useless or causes any financial loss. I see what you say and mean and I agree, but I don't see it higher than low, maybe even info, as I've said in the previous comment.

In addition to that, as I understand MATIC token will still be in use, but later POL will be used as native token instead of MATIC, but the latter will not be depricated or forbidden to use. It will be replaced, but still accepted as a regular ERC20, correct?

Therefore, as I understand it will two different tokens (I mean with two different contracts), but POL will replace MATIC as the native token, correct?

If yes, then I don't see it higher than loe tbh, sorry G

[WangSecurity]

I believe MATIC will just lose its current utility, and will become just a regular ERC20 token. As I've said I can see what you mean, still looks like low/info to me. But will accept any decision from Sherlock

[ZanyBonzy]

Fair enough, I'll also accept any decision, although I still believe its like a proxy token type situation. Thanks for taking the time to listen to my points, @midori-fuse thanks for the esclation.

[Czar102]

Result: Low Unique

I maintain my position and agree with Lead Judge's stance – there is no loss of funds, the contract may fail to prevent users for hurting themselves, nevertheless that doesn't yield a finding severe enough to be considered Medium.

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: rejected