Merge pull request #246 from hyperledger-labs/update-adr-001

Update adr-001 doc

Signed-off-by: Jun Kimura <[email protected]>

docs/adr/adr-001.md

@@ -29,13 +29,42 @@ In order to address these issues, the following three improvements are proposed:
 2. Avoid UpdateClient via the handler: Update LC state directly instead. This reduces the cost of delegatecalls.
 3. Removal of proto-decoding in transaction: Avoid decoding a proto message in a transaction and redefine struct as ethereum ABI.
 
-These improvements are considered possible implementations in ibc-solidity. However, two issues related to IBC-relayer need to be resolved: a. how to resolve the ethereum ABI format from the proto definition and b. how to know the function definition of the LC contract to which the message is sent.
+To implement these in ibc-solidity, the following three points need to be resolved:
 
-To solve these, we decided to add a new helper function `routeUpdateClient` to `ILightClient`. The function decodes the given proto-encoded message, converts it to eth-ABI message and returns it with the call information of the `updateClient` function. The function could be implemented off-chain, such as in a relayer, but considering the reduced complexity of supporting various client implementations and the backwards compatibility with existing proto-encoded `updateClient` messages, it was decided that it should be provided in LC contract.
+a. how to update the commitments in the host(i.e. handler) after updating LC state directly
+b. how to determine the ethereum ABI format from the proto definition
+c. how to know the function definition of the LC contract to which the message is sent
 
-Also, since the relayer can call directly `updateClient` in LC contract without the handler, the responsibility for updating the commitment is delegated to the LC contract. In other words, it is possible to separate the state and commitment updates, so LC contract doesn't need to update the commitments for every UpdateClient.
+To resolve `a`, we added `updateClientCommitments` function to the handler which allows relayers to update the commitments corresponding to the LC state. Since the anyone can call directly `updateClient` in LC contract without the handler, it is required to call this function to set the commitment in the handler after the state is updated. However, in an established channel, there is almost no need to update the commitments every time per `updateClient`. An example implementation of yui-relayer, which does not update client commitments after a connection is established, can be found in the Appendix.
 
-However, please note that the `updateClientCommitments` function, which will be newly added to the handler, allows relayers to update the commitments corresponding to the LC state. However, in an established channel, there is almost no need to update the commitments every time per `updateClient`. An example implementation of yui-relayer, which only update commitments until a connection is established, can be found in the Appendix.
+To resolve `b` and `c`, we added a new helper function `routeUpdateClient` to `ILightClient`. The function decodes the given proto-encoded message, converts it to eth-ABI message and returns it with the call information of the `updateClient` function. The function could be implemented off-chain, such as in a relayer, but considering the backwards compatibility with existing proto-encoded `updateClient` messages, we decide that it should be provided in LC contract. Note that a relayer does not need to use `routeUpdateClient`, assuming it knows the ABI of LC's UpdateClient function.
+
+### updateClientCommitments
+
+Add the `updateClientCommitments` function to the handler to generate the commitment of the updated state in `updateClient`.
+
+```solidity
+function updateClientCommitments(string calldata clientId, Height.Data[] memory heights) public {
+    ILightClient lc = checkAndGetClient(clientId);
+    bytes memory clientState;
+    bytes memory consensusState;
+    bool found;
+    (clientState, found) = lc.getClientState(clientId);
+    require(found, "client not found");
+    commitments[IBCCommitment.clientStateCommitmentKey(clientId)] = keccak256(clientState);
+    for (uint256 i = 0; i < heights.length; i++) {
+        (consensusState, found) = lc.getConsensusState(clientId, heights[i]);
+        require(found, "consensus state not found");
+        bytes32 key = IBCCommitment.consensusStateCommitmentKey(
+            clientId, heights[i].revision_number, heights[i].revision_height
+        );
+        require(commitments[key] == bytes32(0), "consensus state already exists");
+        commitments[key] = keccak256(consensusState);
+    }
+}
+```
+
+`clientId` is the identifier of the target light client instance, and `heights` is a list of heights for updating the commitment.
 
 ### routeUpdateClient
 
@@ -78,32 +107,11 @@ function updateClient(string calldata clientId, Header.Data calldata header) pub
 
 In the above implementation, the `routeUpdateClient` function returns the selector of the public `updateClient` function implemented in the LC contract. Clients such as yui-relayer can send a transaction to execute `updateClient` with the two return values of `routeUpdateClient`.
 
-### updateClientCommitments
-
-Add the `updateClientCommitments` function to the handler to generate the commitment of the updated state in `updateClient`.
+## Security Concerns
 
-```solidity
-function updateClientCommitments(string calldata clientId, Height.Data[] memory heights) public {
-    ILightClient lc = checkAndGetClient(clientId);
-    bytes memory clientState;
-    bytes memory consensusState;
-    bool found;
-    (clientState, found) = lc.getClientState(clientId);
-    require(found, "client not found");
-    commitments[IBCCommitment.clientStateCommitmentKey(clientId)] = keccak256(clientState);
-    for (uint256 i = 0; i < heights.length; i++) {
-        (consensusState, found) = lc.getConsensusState(clientId, heights[i]);
-        require(found, "consensus state not found");
-        bytes32 key = IBCCommitment.consensusStateCommitmentKey(
-            clientId, heights[i].revision_number, heights[i].revision_height
-        );
-        require(commitments[key] == bytes32(0), "consensus state already exists");
-        commitments[key] = keccak256(consensusState);
-    }
-}
-```
+### Constructing Tx using `routeUpdateClient`
 
-`clientId` is the identifier of the target light client instance, and `heights` is a list of heights for updating the commitment.
+If a relayer connects to an untrusted blockchain node and constructs a transaction using the tampered returned value of `routeUpdateClient`, an arbitrary contract call attack can be happened. Therefore, the relayer needs to register a LC contract address and function selectors in the allowed list and ensure that the returned value of  `routeUpdateClient` matches them. However, if the LC contract provides a relayer-trust update function, such a function should not be included within the allowed list. In that case, the relayer should be given the UpdateClient ABI and the relayer should construct an UpdateClient tx instead of using `routeUpdateClient`.
 
 ## Backward Compatibility
 
@@ -198,10 +206,13 @@ func (c *Chain) TxUpdateClient(opts *bind.TransactOpts, msg *clienttypes.MsgUpda
             return err
         }
         // WARNING: if the relayer does not trust the blockchain node, it should check the `lcAddr` and `fnID` with the allowed list
-        calldata := append(fnID[:], args...)
-        return bind.NewBoundContract(lcAddr, abi.ABI{}, c.client, c.client, c.client).RawTransact(opts, calldata)
-    } else {
-        return c.ibcHandler.UpdateClient(opts, m)
+        // For the details, see the Security Concerns section of ADR-001
+        if c.checkAllowList(lcAddr, fnID) {
+            calldata := append(fnID[:], args...)
+            return bind.NewBoundContract(lcAddr, abi.ABI{}, c.client, c.client, c.client).RawTransact(opts, calldata)
+        }
+        // fallback to send a transaction to the handler
     }
+    return c.ibcHandler.UpdateClient(opts, m)
 }
 ```