Race Condition Between Deposits and Fee Changes

[freddyli7]

A deposit could be charged with an unexpected fee if a fee change occurs shortly before the deposit is processed.

Location
Chainsafe_Sygma_Substrate_Pallets/bridge/src/lib.rs#L458

Remediation
only change bridge fees with long lead times, and after giving ample warnings to users.

Security audit team recommend passing the expected fee as a separate argument to the deposit function call, and also recommend refraining from processing the deposit if the actual fee differs from the expectation.

[freddyli7]

split deposit fee as a separate argument of deposit func is considered as a huge breaking change, and I think such issue is avoidable if we take precaution by sending notifications to users and also pause bridge when modifying the fee. Nothing to be fixed or changed in this issue unless we decide to separate the fee, which would be a huge amount of work considering testing

[pesco]

Since I have no insight into your testing process/requirements, I won't argue with that, but codewise this change would be very simple. I can imagine an extra argument for the expected fee and one additional check right before the one that establishes that the incoming amount actually covers the fees. Another thought: Would it be easier to add a new API call ("deposit2" or whatever) that includes the extra check/argument?