Merge pull request #283 from hyperledger-labs/mitigate-reentrancy

Introduce slither

Signed-off-by: Jun Kimura <jun.kimura@datachain.jp>

Author: bluele

.gas-snapshot

@@ -1,20 +1,20 @@
 IBCMockAppTest:testHandshake() (gas: 4420488)
 IBCMockAppTest:testHandshakeBetweenDifferentPorts() (gas: 3334373)
-IBCMockAppTest:testPacketRelay() (gas: 13931365)
+IBCMockAppTest:testPacketRelay() (gas: 13935239)
 IBCMockAppTest:testPacketTimeout() (gas: 4279263)
 IBCTest:testBenchmarkCreateMockClient() (gas: 233366)
 IBCTest:testBenchmarkLCUpdateMockClient() (gas: 62005)
-IBCTest:testBenchmarkRecvPacket() (gas: 158899)
+IBCTest:testBenchmarkRecvPacket() (gas: 158870)
 IBCTest:testBenchmarkSendPacket() (gas: 128432)
 IBCTest:testBenchmarkUpdateMockClient() (gas: 160229)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947)
-TestICS02:testCreateClient() (gas: 36551707)
-TestICS02:testInvalidCreateClient() (gas: 36448926)
-TestICS02:testInvalidUpdateClient() (gas: 36447834)
-TestICS02:testRegisterClient() (gas: 36103500)
-TestICS02:testRegisterClientDuplicatedClientType() (gas: 36088809)
-TestICS02:testRegisterClientInvalidClientType() (gas: 36118270)
-TestICS02:testUpdateClient() (gas: 36616034)
+TestICS02:testCreateClient() (gas: 36496843)
+TestICS02:testInvalidCreateClient() (gas: 36394062)
+TestICS02:testInvalidUpdateClient() (gas: 36392970)
+TestICS02:testRegisterClient() (gas: 36048636)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 36033945)
+TestICS02:testRegisterClientInvalidClientType() (gas: 36063406)
+TestICS02:testUpdateClient() (gas: 36561170)
 TestICS03Handshake:testConnOpenAck() (gas: 1858230)
 TestICS03Handshake:testConnOpenConfirm() (gas: 2054143)
 TestICS03Handshake:testConnOpenInit() (gas: 1429838)
@@ -37,32 +37,32 @@ TestICS04Handshake:testChanOpenInit() (gas: 2543524)
 TestICS04Handshake:testChanOpenTry() (gas: 3099898)
 TestICS04Handshake:testInvalidChanOpenAck() (gas: 2439749)
 TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2517338)
-TestICS04Handshake:testInvalidChanOpenInit() (gas: 1758704)
-TestICS04Handshake:testInvalidChanOpenTry() (gas: 1773674)
-TestICS04Packet:testAcknowledgementPacket() (gas: 3351152)
+TestICS04Handshake:testInvalidChanOpenInit() (gas: 1760558)
+TestICS04Handshake:testInvalidChanOpenTry() (gas: 1775528)
+TestICS04Packet:testAcknowledgementPacket() (gas: 3351116)
 TestICS04Packet:testInvalidSendPacket() (gas: 3551583)
-TestICS04Packet:testRecvPacket() (gas: 10995054)
+TestICS04Packet:testRecvPacket() (gas: 10996130)
 TestICS04Packet:testRecvPacketTimeoutHeight() (gas: 3259727)
 TestICS04Packet:testRecvPacketTimeoutTimestamp() (gas: 3278877)
 TestICS04Packet:testSendPacket() (gas: 6412442)
 TestICS04Packet:testTimeoutOnClose() (gas: 3553289)
-TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46737910)
-TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3455585)
-TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266374)
-TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5235544)
-TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4405811)
-TestICS04Upgrade:testUpgradeFull() (gas: 57775990)
-TestICS04Upgrade:testUpgradeInit() (gas: 3068711)
-TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471930)
-TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3902151)
-TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5238138)
-TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5612071)
-TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4070658)
-TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17677097)
-TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21313205)
-TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 44260340)
-TestICS04Upgrade:testUpgradeToOrdered() (gas: 56489888)
-TestICS04Upgrade:testUpgradeToUnordered() (gas: 45099675)
+TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46742335)
+TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3456630)
+TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266005)
+TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236258)
+TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4407259)
+TestICS04Upgrade:testUpgradeFull() (gas: 57784728)
+TestICS04Upgrade:testUpgradeInit() (gas: 3069408)
+TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471936)
+TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3903220)
+TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5237770)
+TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5610957)
+TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071025)
+TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17681581)
+TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21317741)
+TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 44264077)
+TestICS04Upgrade:testUpgradeToOrdered() (gas: 56509529)
+TestICS04Upgrade:testUpgradeToUnordered() (gas: 45113829)
 TestICS04UpgradeApp:testUpgradeAuthorizationChanneNotFound() (gas: 61690)
 TestICS04UpgradeApp:testUpgradeAuthorizationRePropose() (gas: 2565532)
 TestICS04UpgradeApp:testUpgradeAuthorizationRemove() (gas: 2473992)

.github/workflows/test.yml

@@ -47,6 +47,17 @@ jobs:
     - name: Run tests with minimal solidity version
       run: make SOLC_VERSION=${{ env.MINIMAL_SOLC_VERSION }} test
 
+  slither:
+    name: Slither analysis
+    needs: contract-test
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: crytic/slither-action@v0.4.0
+        with:
+          node-version: 20.13
+          slither-version: 0.10.1
+
   e2e-test:
     name: E2E test
     needs: contract-test

Makefile

@@ -29,6 +29,10 @@ test:
 coverage:
 	@forge coverage --use solc:$(SOLC_VERSION)
 
+.PHONY: slither
+slither:
+	@slither .
+
 ######## Protobuf ########
 
 .PHONY: proto-sol

contracts/core/02-client/ILightClient.sol

@@ -78,6 +78,7 @@ interface ILightClient {
     /**
      * @dev verifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the specified height.
      * The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
+     * This function should not perform `call` to the IBC contract. However, `staticcall` is permitted.
      */
     function verifyMembership(
         string calldata clientId,
@@ -93,6 +94,7 @@ interface ILightClient {
     /**
      * @dev verifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at a specified height.
      * The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
+     * This function should not perform `call` to the IBC contract. However, `staticcall` is permitted.
      */
     function verifyNonMembership(
         string calldata clientId,

contracts/core/03-connection/IBCConnection.sol

@@ -208,6 +208,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
         bytes memory proof,
         bytes memory clientStateBytes
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -220,6 +221,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
                 clientStateBytes
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCConnectionFailedVerifyClientState(connection.client_id, path, clientStateBytes, proof, height);
@@ -232,6 +234,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
         bytes memory proof,
         bytes memory consensusStateBytes
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -246,6 +249,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
                 consensusStateBytes
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCConnectionFailedVerifyClientConsensusState(
@@ -266,6 +270,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
         string memory counterpartyConnectionId,
         ConnectionEnd.Data memory counterpartyConnection
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -278,6 +283,7 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
                 ConnectionEnd.encode(counterpartyConnection)
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCConnectionFailedVerifyConnectionState(

contracts/core/04-channel/IBCChannelHandshake.sol

@@ -20,11 +20,14 @@ import {IBCChannelLib} from "./IBCChannelLib.sol";
 contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChannelErrors {
     using IBCHeight for Height.Data;
 
+    // ------------- IIBCChannelHandshake implementation ------------------- //
+
     /**
      * @dev channelOpenInit is called by a module to initiate a channel opening handshake with a module on another chain.
      */
     function channelOpenInit(IIBCChannelHandshake.MsgChannelOpenInit calldata msg_)
-        external
+        public
+        override
         returns (string memory, string memory)
     {
         if (msg_.channel.connection_hops.length != 1) {
@@ -52,6 +55,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
 
         string memory channelId = generateChannelIdentifier();
         initializeSequences(msg_.portId, channelId);
+        emit GeneratedChannelIdentifier(channelId);
 
         IIBCModule module = lookupModuleByPort(msg_.portId);
         string memory version = module.onChanOpenInit(
@@ -74,15 +78,15 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
             msg_.channel.connection_hops,
             version
         );
-        emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }
 
     /**
      * @dev channelOpenTry is called by a module to accept the first step of a channel opening handshake initiated by a module on another chain.
      */
     function channelOpenTry(IIBCChannelHandshake.MsgChannelOpenTry calldata msg_)
-        external
+        public
+        override
         returns (string memory, string memory)
     {
         if (msg_.channel.connection_hops.length != 1) {
@@ -127,6 +131,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
 
         string memory channelId = generateChannelIdentifier();
         initializeSequences(msg_.portId, channelId);
+        emit GeneratedChannelIdentifier(channelId);
 
         IIBCModule module = lookupModuleByPort(msg_.portId);
         string memory version = module.onChanOpenTry(
@@ -149,14 +154,13 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
             msg_.channel.connection_hops,
             version
         );
-        emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }
 
     /**
      * @dev channelOpenAck is called by the handshake-originating module to acknowledge the acceptance of the initial request by the counterparty module on the other chain.
      */
-    function channelOpenAck(IIBCChannelHandshake.MsgChannelOpenAck calldata msg_) external {
+    function channelOpenAck(IIBCChannelHandshake.MsgChannelOpenAck calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state != Channel.State.STATE_INIT) {
             revert IBCChannelUnexpectedChannelState(channel.state);
@@ -195,7 +199,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
     /**
      * @dev channelOpenConfirm is called by the counterparty module to close their end of the channel, since the other end has been closed.
      */
-    function channelOpenConfirm(IIBCChannelHandshake.MsgChannelOpenConfirm calldata msg_) external {
+    function channelOpenConfirm(IIBCChannelHandshake.MsgChannelOpenConfirm calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state != Channel.State.STATE_TRYOPEN) {
             revert IBCChannelUnexpectedChannelState(channel.state);
@@ -227,7 +231,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
     /**
      * @dev channelCloseInit is called by either module to close their end of the channel. Once closed, channels cannot be reopened.
      */
-    function channelCloseInit(IIBCChannelHandshake.MsgChannelCloseInit calldata msg_) external {
+    function channelCloseInit(IIBCChannelHandshake.MsgChannelCloseInit calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state == Channel.State.STATE_UNINITIALIZED_UNSPECIFIED) {
             revert IBCChannelChannelNotFound(msg_.portId, msg_.channelId);
@@ -249,7 +253,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
      * @dev channelCloseConfirm is called by the counterparty module to close their end of the
      * channel, since the other end has been closed.
      */
-    function channelCloseConfirm(IIBCChannelHandshake.MsgChannelCloseConfirm calldata msg_) external {
+    function channelCloseConfirm(IIBCChannelHandshake.MsgChannelCloseConfirm calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state == Channel.State.STATE_UNINITIALIZED_UNSPECIFIED) {
             revert IBCChannelChannelNotFound(msg_.portId, msg_.channelId);
@@ -285,6 +289,8 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         );
     }
 
+    // ------------- Private functions ------------------- //
+
     /**
      * @dev writeChannel writes a channel which has successfully passed the OpenInit or OpenTry handshake step.
      */
@@ -296,7 +302,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         ChannelCounterparty.Data calldata counterparty,
         string[] calldata connectionHops,
         string memory version
-    ) internal {
+    ) private {
         Channel.Data storage channel = channels[portId][channelId];
         channel.state = state;
         channel.ordering = order;
@@ -309,13 +315,20 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         updateChannelCommitment(portId, channelId);
     }
 
+    function initializeSequences(string memory portId, string memory channelId) internal {
+        nextSequenceSends[portId][channelId] = 1;
+        nextSequenceRecvs[portId][channelId] = 1;
+        nextSequenceAcks[portId][channelId] = 1;
+        recvStartSequences[portId][channelId].sequence = 1;
+        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(portId, channelId)] =
+            keccak256(abi.encodePacked((bytes8(uint64(1)))));
+    }
+
     function updateChannelCommitment(string memory portId, string memory channelId) private {
         commitments[IBCCommitment.channelCommitmentKey(portId, channelId)] =
             keccak256(Channel.encode(channels[portId][channelId]));
     }
 
-    /* Verification functions */
-
     function verifyChannelState(
         ConnectionEnd.Data storage connection,
         Height.Data calldata height,
@@ -324,6 +337,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         string memory channelId,
         bytes memory channelBytes
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -336,24 +350,14 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
                 channelBytes
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCChannelFailedVerifyChannelState(
             connection.client_id, IBCCommitment.channelPath(portId, channelId), channelBytes, proof, height
         );
     }
 
-    /* Internal functions */
-
-    function initializeSequences(string memory portId, string memory channelId) internal {
-        nextSequenceSends[portId][channelId] = 1;
-        nextSequenceRecvs[portId][channelId] = 1;
-        nextSequenceAcks[portId][channelId] = 1;
-        recvStartSequences[portId][channelId].sequence = 1;
-        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(portId, channelId)] =
-            keccak256(abi.encodePacked((bytes8(uint64(1)))));
-    }
-
     function getCounterpartyHops(string memory connectionId) internal view returns (string[] memory hops) {
         hops = new string[](1);
         hops[0] = connections[connectionId].counterparty.connection_id;