search

tokamak-network / crossTrade

Cross Trade is a new core service for optimistic rollups that complements standard withdrawals and fast withdrawals. It is trustless and do not require extensive backend.
4 stars 1 forks source link

[security vulnerability] A cross trade can be blocked by attackers #31

Closed nguyenzung closed 3 months ago

nguyenzung commented 3 months ago

Describe the bug Attacker can call provideCT with very small _fwAmount. So on L1, the CT is record as a successful trading => no more doing: trade, edit, cancel. l2's token will also be blocked on L2

Configuration

Impact A CrossTrade can be blocked

Recommendation We can check _fwAmount

Exploit Scenario

Demo

zzooppii commented 3 months ago

This seems to overlap with issue29.

zzooppii commented 3 months ago

As a solution to issue29, I tried to solve it by saving fwAmount to storage. Would it be okay?

nguyenzung commented 3 months ago

Both issue29 and issue31 are related to _fwAmount, but there are 2 scenarios and there are solutions can solve #29 but can not solve #31.

I think saving fwAmount can solve #29 but can not solve this issue. Because L1 does not know the correct _fwAmount on L2 an L1CrossTrade accepts any _fwAmount if there is no edit.

I think if there are solutions, we also need to check carefully to make sure there is no side effect.

zzooppii commented 3 months ago

@suahnkim This issue and issue #29 seem to be the same. What do you think? With this solution, I would like to create a separate provide function for when fwAmount must eventually be entered as a hash value and is edited.

suahnkim commented 3 months ago

yes, I think the hash must contain initial_fwAmount, during edit, we probably should use a different storage so that the hash does not change based on the updated_fwAmount

suahnkim commented 3 months ago

thank you @nguyenzung

suahnkim commented 3 months ago

@nguyenzung the reason why fwAmount was omitted in the hash calculation was that fwAmount can be modified via edit, causing a hash mismatch on L1 and L2, but I think we forgot to account for basic requirement that only requester can set fwAmount and it has to be verified

suahnkim commented 3 months ago

This issue is related to #29, closing this issue