[checking: security vulnerability] Function claimCT can be called with incorrect msg.sender

[nguyenzung]

Describe the bug Missing checking msg.sender at this line https://github.com/tokamak-network/crossTrade/blob/codeReview/contracts/L2/L2CrossTrade.sol#L53 Attackers can call L2CrossDomainMessenger.relayMessage in Thanos with correct parameters to make a call to L2CrossTrade.claimCT, so that they can steal all token.

This function is safe in Titan because only l1CrossDomainMessenger can call

Configuration

• Severity: SUGGESTION

Impact Users lost their token

Recommendation Adding check: msg.sender == address(crossDomainMessenger) in the modifier checkL1()

Exploit Scenario

Demo

[zzooppii]

Thank you for your good comments!

So does this mean that Thanos can manipulate the xDomainMessageSender value?

[nguyenzung]

Thank you for your good comments!

So does this mean that Thanos can manipulate the xDomainMessageSender value?

Thanos inherits relayMessage from Optimism's Bedrock/Ecotone. In Thanos/current Optimism, relayMessage can be called again for failed messages by L2's users.

Actually, I see that it even seems safe with the current checkL1 in Thanos/Optimism. But in Optimism, it usually checks like: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/universal/StandardBridge.sol#L107

Maybe I think it is better if we update the check like that. Let me update the label of this issue

[zzooppii]

Thank you for your good comments! It would be safer to change it as you said.

[parth-15]

I had thought of same but couldn't find any way on how can require( IL2CrossDomainMessenger(crossDomainMessenger) .xDomainMessageSender() == chainData[_chainId].l1CrossTradeContract, "only call l1FastWithdraw" ); can fail. But I agree that it is good suggestion to be extra careful.

[zzooppii]

Thank you for your good interest :thumbsup:

[nguyenzung]

I think in case there are more than one deployed L2CrossTrade that mapping with the same L1CrossTrade .

When the function claimCT() is called in one L2CrossTrade, it is possible to trigger a call to claimCT in another L2CrossTrade with https://github.com/tokamak-network/crossTrade/blob/codeReview_Apply/contracts/L1/L1CrossTrade.sol#L90 and the triggered call can pass every checking conditions.

It also depend on the operators, but I think this scenario can occur on L2. So that the Optimism team has updated the way checking in cases like our cases

@parth-15 @zzooppii

[parth-15]

@nguyenzung I don't think if it's possible here since there is mapping from chainId to l1CrossTradeContract. https://github.com/tokamak-network/crossTrade/blob/codeReview/contracts/L2/L2CrossTrade.sol#L53-L59

But I still agree with your comment that check is useful for extra security.

[nguyenzung]

@parth-15 I mean 2 L2CrossTrade can have the same chainData[_chainId].l1CrossTradeContract.

[zzooppii]

@nguyenzung Yes, that's right For example, we plan to use one L1 CrossTradeContract, Thanos L2 CrossTradeContract, and Titan L2 CrossTradeContract. Could this be a problem?

Here, Thanos' chainData[_l2chainId].legacyERC20ETH information and Titan's chainData[_l2chainId].legacyERC20ETH information will be different, so isn't it okay?

[parth-15]

@parth-15 I mean 2 L2CrossTrade can have the same chainData[_chainId].l1CrossTradeContract.

But L1CrossTradeContract also have chainId and address mapping. So, i don't think it's exploitable here. But still better for safety.

[nguyenzung]

Thanos L2 CrossTradeContract

I think it seems be ok now. I think the problem could happen if we make a update by using another address Thanos L2 CrossTrade. But if checking msg.sender == address(crossDomainMessenger) => it is safe for this case

[zzooppii]

@parth-15 @nguyenzung thank you For your comment!