search

hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

`set_committee` Implementation will lead to freezing of user funds #73

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: -- Twitter username: 0xleadwizard Submission hash (on-chain): 0x584acb0846aacbf42876b496bd540e7b25649dd87f73f963516517d54d0d1a1b Severity: high

Description: Description\

set_committee function Implementation on Aleph differs from the implementation deployed on ETH.

The contract deployed on Aleph deletes members from the previous committee_id and only stores the new ones with committee_id+1.

This causes issues as on ETH there could be a transaction pending with the old committee_id or if the contract deployed on ETH is not upgraded to a new committee id, all the transactions that happen during this period will result in the locking of the user funds with no bridgin on Alpeh chain!

When the relayer tries to bridge these transactions with old committee IDs to Alpeh, the relayer will call receive_request function with old committee_id, but it will fail as it calls only_committee_member function which fails due to the committee_id passed in the function not matching the current committee_id that is stored in self.committees

Recommendation\

Implementation of set_committee function on Aleph instead of creating new committee set, should append the old committee set with new data!

fbielejec commented 4 months ago

Generally a working PoC is required for submission to be considered valid, we will have an internal discussion about this issue as new submissions are closed now.

Jonsey commented 4 months ago

I also saw this issue and couldn't understand why my tests were passing when I changed committee and tested receive_request with the previous committee id. Turns out the big issue here is that only_committee_member returns false not Err if the key does not exist. So does not actually work.

Shame I only just worked this out

Jonsey commented 4 months ago

Hence the need for a POC

krzysztofziobro commented 4 months ago

While the code is quite confusing it actually works as intended due to the way Mappings are represented in storage + how type inference in Rust works.

So there is no vulnerability.