movementlabsxyz / movement

The Movement Network is a Move-based L2 on Ethereum.
Apache License 2.0
82 stars 66 forks source link

Add Bridge Counterparty Hash Verification #822

Closed Primata closed 2 weeks ago

Primata commented 2 weeks ago

Summary

counterparty has to hash the bridgeTransferId components to check if its produced by the correct parameters For that to happen two things have to be added. One is the nonce on initiator has to be emitted and used as a parameter in lock call Two the initiate block.timestamp has to be emitted and used as a parameter in lock call With that we can produce the same hash on both sides

Changelog

Added the requirements above.

Testing

Not tested

Outstanding issues

Testing

franck44 commented 2 weeks ago

I am not a reviewer but I an arguing against this PR:

Overall, these unjustified changes do not improve the security of the bridge but rather create more problems (sync of the nonces).

Primata commented 2 weeks ago

@franck44 see that this PR accompanies a PR to aptos-core, it would also update the bridge move modules.

  1. By hashing the same values, Counterparty can verify that the lockBridgeTransfer call is using the correct parameters. This prevents bugs where the Relayer inputs incorrect values.
  2. I can improve on why they should be implemented.
  3. Sides of the bridge are initiator and counterparty. The nonce is emitted by the initiator as is to be used on the counterparty. Same thing for the initiateTimestamp.
  4. By also using initiateTimestamp in the keccak256, lock sets time_lock to a point in where it prevents the exploit I described yesterday that we discussed. It pairs initiator and counterparty starting time.
franck44 commented 2 weeks ago

@franck44 see that this PR accompanies a PR to aptos-core, it would also update the bridge move modules.

  1. By hashing the same values, Counterparty can verify that the lockBridgeTransfer call is using the correct parameters. This prevents bugs where the Relayer inputs incorrect values.
  2. I can improve on why they should be implemented.
  3. Sides of the bridge are initiator and counterparty. The nonce is emitted by the initiator as is to be used on the counterparty. Same thing for the initiateTimestamp.
  4. By also using initiateTimestamp in the keccak256, lock sets time_lock to a point in where it prevents the exploit I described yesterday that we discussed. It pairs initiator and counterparty starting time.

@0xPrimata A PR for this potential problem is premature and we may move this discussion into an issue to discuss the impact of the changes first.

andyjsbell commented 2 weeks ago

Agreed, please create issue @0xPrimata

I would also vote on closing this PR until we have consensus on this in the issue