roaminro / koinos-bridge-contract

MIT License
4 stars 2 forks source link

Bridge Contract Feedback #2

Open mvandeberg opened 1 year ago

mvandeberg commented 1 year ago

I have read through the Koinos side of the bridge. I am assuming the Ethereum side has an identical interface. This feedback is not conclusive, nor is it based on the Ethereum implementation.

Why is direction an integer? Why not make it a boolean?

Should initialization be required in any of the entry points? For example, if you upload and don't immediately initialize, anyone can add themselves as a validator. Likewise, initialization should probably require the contract's authority.

Mint permissions is required for wrapped tokens. Is that problematic? Not necessarily bad, but I think this is a design decision that could use further discussion.

Wrapped tokens "belong" to the remote chain. Non wrapped tokens "belong" to Koinos. Is there a way to abstract this?

Is the meta nonce asserted anywhere?

Regarding the multisig requirements of validators.

The current equation results in the following:

Have, Require, Ratio:
   1,       1, 1     (1)
   2,       2, 1     (1)
   3,       3, 1     (1)
   4,       3, 3/4   (0.75)
   5,       4, 4/5   (0.8)
   6,       5, 5/6   (0.833)
   7,       5, 5/7   (0.714)
   8,       6, 3/4   (0.75)
   9,       7, 7/9   (0.777)
  10,       7, 7/10  (0.7)
  11,       8, 8/11  (0.727)
  12,       9, 3/4   (0.75)
  13,       9, 9/13  (0.692)
  14,      10, 5/7   (0.714)
  15,      11, 11/15 (0.733)
  16,      11, 11/16 (0.687)
  17,      12, 12/17 (0.705)
  18,      13, 13/18 (0.722)
  19,      13, 13/19 (0.684)
  20,      14, 7/10  (0.7)

The requirement does increase by 2 for every 3. So At infinity, I agree this would be a 2:3 multisig.

For smaller numbers of validators, the actual requirement is sometimes much higher (5 of 6 instead of 4 of 6).

Is that a desirable property, or would defining requirements as (nbValidators * 2 + 2) / 3 be sufficient?

Proposed Change:

Have, Require, Ratio:
   1,       1, 1     1
   2,       2, 1     1
   3,       2, 2/3   0.66
   4,       3, 3/4   0.75
   5,       4, 4/5   0.8
   6,       4, 2/3   0.66
   7,       5, 5/7   0.714
   8,       6, 3/4   0.75
   9,       6, 2/3   0.66
  10,       7, 7/10  0.7
  11,       8, 8/11  0.727
  12,       8, 2/3   0.66
  13,       9, 9/13  0.692
  14,      10, 5/7   0.714
  15,      10, 2/3   0.66
  16,      11, 11/16 0.687
  17,      12, 12/17 0.705
  18,      12, 2/3   0.66
  19,      13, 13/19 0.684
  20,      14, 7/10  0.7

Probably don't want to do base58 encoding unless there is an error. It can be expensive. (https://github.com/roaminro/koinos-bridge-contract/blob/2712eebb79d1cfeed4db3fcd65f3773b5be71e0f/assembly/Bridge.ts#L450)

What is the purpose of request_new_signature?

Is there a way for the user to get their tokens back after a certain amount of time? It would appear that complete_transfer could release the funds back to the user. This seems like the biggest potential problem for the validators running the bridge. Because the tokens are immediately transferred or burned, the chain of custody pretty clearly leads through the validators, with no recourse for the user if the validators make a mistake. Having many validators certainly breaks up this custodial responsibility, but does not remove it? Is this enough to support non-custodial status of validators?

roaminro commented 1 year ago

Thanks for the feedback @mvandeberg!

Why is direction an integer? Why not make it a boolean?

Good point, I changed the direction type to boolean

Should initialization be required in any of the entry points? For example, if you upload and don't immediately initialize, anyone can add themselves as a validator. Likewise, initialization should probably require the contract's authority.

Good point, "initialize" now requires the contract's authority

Mint permissions is required for wrapped tokens. Is that problematic? Not necessarily bad, but I think this is a design decision that could use further discussion.

I'm not sure what you mean, the bridge contract needs to have full control of the wrapped tokens, everytime you want to add a bridged token to Koinos, you need to deploy a token contract that has the mint/burn functions requiring the bridge token authority

Wrapped tokens "belong" to the remote chain. Non wrapped tokens "belong" to Koinos. Is there a way to abstract this?

Right, there's probably a better way to abstract this, I basically converted the solidity contract into an AS contract.

Is the meta nonce asserted anywhere?

Yes, it is indirectly checked everytime a function that's not transfer/complete_transfer is called. (it is part of the hash that is signed by the validators)

Regarding the multisig requirements of validators.

Agreed, I used the formula from Wormhole, we can definitely use yours

Probably don't want to do base58 encoding unless there is an error. It can be expensive.

Right, I updated the contract accordingly

What is the purpose of request_new_signature?

All the signatures generated by the validators have an expiration time (this can be set in the validator node, currently set to 1hr), in case of a hack, they could pause the bridge and wait for the signatures to expire. During normal operation, if you don't submit the signatures and they expire, you can request new ones by calling the request signatures function. This will generate an event that is read by the nodes. You can only request new signatures after 2x expiration time (2 hrs currently)

Regarding the last item, we'd need to burn some brain cells to figure out a wa y to improve it, I currently have not found a way that doesn't require validators to post transactions on chain. Currently, the complete transfers are done by the end users directly, validators are only streaming the chains and emitting sigantures off-chain.

Thanks again for taking the time to look into the contract!