search

icon-project / icon-bridge

The centralized bridge of ICON
Apache License 2.0
21 stars 15 forks source link

Scope most sensitive parts of code base for audit #18

Closed CyrusVorwald closed 2 years ago

CyrusVorwald commented 2 years ago

Overview

We want to identify the order in which the security auditors should review our code base.

Rationale

Because the audit for the full scope of the ICON Bridge repository will take an estimated 83 days, we want to identify the most sensitive parts of the project first. This will reduce the time we are most exposed to risk.

Acceptance Criteria

donghyun89 commented 2 years ago

Since there is no BMV in Bridge, BMC should recognize that the message come from BMR which is set already. So code audit should cover the code that BMC can recognize the origin of RelayMessage. https://github.com/icon-project/icon-bridge/blob/f5d98e56b71a3ba24cfee176bf640406323e2aa3/javascore/bmc/src/main/java/foundation/icon/btp/bmc/BTPMessageCenter.java#L328

bbist commented 2 years ago

Important parts to secure:

  1. BMC owner (admin) wallet should be secured with utmost care. If it's compromised, entire protocol is at a risk. It's best to have a separate deployer from the owner, to be able to upgrade the contracts even if the key gets compromised. Alternatively, we can use a simple multi-sig (can be stripped down minimal multi-sig) contract to deploy as well as manage the contracts. Idea can be taken from ICON governance contract that set parameters for other projects.
  2. BTPTokenService, ie. fund transfers (refunds) should be audited thoroughly once all the e2etests pass.
  3. Relayer should implement a minimal verifier for each chain integrated into the icon-bridge. This helps avoid endpoint hijacking as well as intentionally fraudulent nodes, including the third party RPC endpoints that we might use. It's necessary for the trusted relayer, and is optional for BTP 2.0, trustless relayer.
  4. Relayer, contract deployer, and admin keys should be different, to minimize the risk of losing control of multiple components at once.
  5. Might make sense to make the contracts pausable.
bbist commented 2 years ago

@sdpisreddevil, If you have some ideas you can share it here. Any potential issues that we need to be aware of on Javascore side.

bbist commented 2 years ago

@manishbista28, Are the verifiers implemented correctly in Relayer? I mean as intended by the source chain consensus.

donghyun89 commented 2 years ago

It needs to be reviewed that we get the wallet information by calling Wallet() and inject it to Sender object when the Relay server is starting. Once it is created, whenever the wallet information is needed, it just use it from the memory and there is no chance to remove it while server is up and running. Is it safe to do that way or it needs to fetch the wallet information when it is needed and remove it in memory right away? https://github.com/icon-project/icon-bridge/blob/4be045ed983279171c61b9d860f66a11982e48fd/cmd/btpsimple/relay/multi_relay.go#L42

bbist commented 2 years ago

Is it safe to do that way or it needs to fetch the wallet information when it is needed and remove it in memory right away?

It makes sense to dynamically fetch the wallet information from AWS and remove it as soon as a transaction is signed. It reduces the overall duration of private key being exposed via memory inspection. We also need to ensure that the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are secured and cross-account secrets access in AWS is disabled.

donghyun89 commented 2 years ago

Every Java score has its own access control, for example, https://github.com/icon-project/icon-bridge/blob/4be045ed983279171c61b9d860f66a11982e48fd/javascore/bsh/src/main/java/foundation/icon/btp/bsh/ServiceHandler.java#L88 it needs to verify the access control works well in any cases in every contract

donghyun89 commented 2 years ago

@bbist Does Relay check BMC address in source chain? For example, ICON receiver check eventlog signature whether it is BTP message or not. But we have to make sure that the eventlog should be made by only ICON BMC, not faked one. Right?

bbist commented 2 years ago

This is in reference to a possible issue mentioned in https://github.com/icon-project/icon-bridge/discussions/68#discussioncomment-3195931.

This is true @MuhammedIrfan.

The token name is the identifier that has to be unique. A token registration takes token name and symbol, that allows for conflicting symbols as long as the name is unique across chains. Therefore, if convention is followed to keep track of registered tokens across all chains, this can be handled.

But the underlying issue that you're alluding to remains valid. If someone gets the admin access to contracts in any of the connected chains, then funds can be drained at the source chain by simply registering a token with identical name and infinite supply.

This is the reason why we've also been discussing about the need for multi-sig wallet for admin controls. https://github.com/icon-project/icon-bridge/issues/32, https://github.com/icon-project/icon-bridge/issues/33, and https://github.com/icon-project/icon-bridge/issues/34.

bbist commented 2 years ago

@bbist Does Relay check BMC address in source chain? For example, ICON receiver check eventlog signature whether it is BTP message or not. But we have to make sure that the eventlog should be made by only ICON BMC, not faked one. Right?

As far as I remember, the BMC address check has been implemented in Relay for Harmony and ICON, and not for BSC. The BSC implementation also lacks verification, header as well as receipt proofs on Relay. It uses ethereum.FilterQuery to directly fetch logs for specified BMC contract address. I suppose that's fine for the beta release, as hinted by @CyrusVorwald-ICON in https://github.com/icon-project/icon-bridge/issues/35#issuecomment-1179198611. But this will be addressed formally when the verification is implemented.