introduce slither and clarify some trusted calls

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

Author: bluele

.gas-snapshot

@@ -8,13 +8,13 @@ IBCTest:testBenchmarkRecvPacket() (gas: 158899)
 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: 36498151)
+TestICS02:testInvalidCreateClient() (gas: 36395370)
+TestICS02:testInvalidUpdateClient() (gas: 36394278)
+TestICS02:testRegisterClient() (gas: 36049944)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 36035253)
+TestICS02:testRegisterClientInvalidClientType() (gas: 36064714)
+TestICS02:testUpdateClient() (gas: 36562478)
 TestICS03Handshake:testConnOpenAck() (gas: 1858230)
 TestICS03Handshake:testConnOpenConfirm() (gas: 2054143)
 TestICS03Handshake:testConnOpenInit() (gas: 1429838)
@@ -46,23 +46,23 @@ 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: 46742362)
+TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3456630)
+TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266752)
+TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236270)
+TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4407259)
+TestICS04Upgrade:testUpgradeFull() (gas: 57784890)
+TestICS04Upgrade:testUpgradeInit() (gas: 3069408)
+TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471936)
+TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3903220)
+TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5238516)
+TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5612449)
+TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071036)
+TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17681581)
+TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21317741)
+TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 44264108)
+TestICS04Upgrade:testUpgradeToOrdered() (gas: 56493668)
+TestICS04Upgrade:testUpgradeToUnordered() (gas: 45102699)
 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/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

@@ -324,6 +324,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,6 +337,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
                 channelBytes
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCChannelFailedVerifyChannelState(

contracts/core/04-channel/IBCChannelPacketSendRecv.sol

@@ -310,6 +310,7 @@ contract IBCChannelPacketSendRecv is
         bytes memory path,
         bytes32 commitment
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -322,6 +323,7 @@ contract IBCChannelPacketSendRecv is
                 abi.encodePacked(commitment)
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCChannelFailedVerifyPacketCommitment(connection.client_id, path, commitment, proof, height);
@@ -334,6 +336,7 @@ contract IBCChannelPacketSendRecv is
         bytes memory path,
         bytes32 acknowledgementCommitment
     ) private {
+        // slither-disable-start reentrancy-no-eth
         if (
             checkAndGetClient(connection.client_id).verifyMembership(
                 connection.client_id,
@@ -346,6 +349,7 @@ contract IBCChannelPacketSendRecv is
                 abi.encodePacked(acknowledgementCommitment)
             )
         ) {
+            // slither-disable-end reentrancy-no-eth
             return;
         }
         revert IBCChannelFailedVerifyPacketAcknowledgement(

contracts/core/04-channel/IBCChannelPacketTimeout.sol

@@ -75,6 +75,7 @@ contract IBCChannelPacketTimeout is IBCModuleManager, IIBCChannelPacketTimeout,
                 revert IBCChannelPacketMaybeAlreadyReceived(msg_.packet.sequence, msg_.nextSequenceRecv);
             }
             if (
+                // slither-disable-next-line reentrancy-no-eth
                 !client.verifyMembership(
                     connection.client_id,
                     msg_.proofHeight,
@@ -104,6 +105,7 @@ contract IBCChannelPacketTimeout is IBCModuleManager, IIBCChannelPacketTimeout,
                 msg_.packet.destinationPort, msg_.packet.destinationChannel, msg_.packet.sequence
             );
             if (
+                // slither-disable-next-line reentrancy-no-eth
                 !client.verifyNonMembership(
                     connection.client_id,
                     msg_.proofHeight,
@@ -188,6 +190,7 @@ contract IBCChannelPacketTimeout is IBCModuleManager, IIBCChannelPacketTimeout,
                 upgrade_sequence: msg_.counterpartyUpgradeSequence
             });
             if (
+                // slither-disable-next-line reentrancy-no-eth
                 !client.verifyMembership(
                     connection.client_id,
                     msg_.proofHeight,
@@ -215,6 +218,7 @@ contract IBCChannelPacketTimeout is IBCModuleManager, IIBCChannelPacketTimeout,
                 revert IBCChannelPacketMaybeAlreadyReceived(msg_.packet.sequence, msg_.nextSequenceRecv);
             }
             if (
+                // slither-disable-next-line reentrancy-no-eth
                 !client.verifyMembership(
                     connection.client_id,
                     msg_.proofHeight,
@@ -243,6 +247,7 @@ contract IBCChannelPacketTimeout is IBCModuleManager, IIBCChannelPacketTimeout,
                 msg_.packet.destinationPort, msg_.packet.destinationChannel, msg_.packet.sequence
             );
             if (
+                // slither-disable-next-line reentrancy-no-eth
                 !client.verifyNonMembership(
                     connection.client_id,
                     msg_.proofHeight,

contracts/core/04-channel/IBCChannelUpgrade.sol

@@ -282,13 +282,15 @@ abstract contract IBCChannelUpgradeBase is
         bytes memory path,
         bytes memory value
     ) internal {
+        // slither-disable-start reentrancy-no-eth
         if (
             ILightClient(clientImpls[connection.client_id]).verifyMembership(
                 connection.client_id, height, 0, 0, proof, connection.counterparty.prefix.key_prefix, path, value
             )
         ) {
             return;
         }
+        // slither-disable-end reentrancy-no-eth
         revert IBCChannelUpgradeFailedVerifyMembership(connection.client_id, string(path), value, proof, height);
     }
 
@@ -332,7 +334,7 @@ abstract contract IBCChannelUpgradeBase is
                 counterpartyChannel
             );
         }
-
+        // slither-disable-next-line reentrancy-no-eth
         verifyMembership(
             connection,
             proofs.proofHeight,
@@ -373,7 +375,9 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
         channel.upgrade_sequence++;
         updateChannelCommitment(msg_.portId, msg_.channelId, channel);
 
-        upgrade.fields = msg_.proposedUpgradeFields;
+        upgrade.fields.ordering = msg_.proposedUpgradeFields.ordering;
+        upgrade.fields.connection_hops = new string[](1);
+        upgrade.fields.connection_hops[0] = msg_.proposedUpgradeFields.connection_hops[0];
         upgrade.fields.version = lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeInit(
             msg_.portId, msg_.channelId, channel.upgrade_sequence, msg_.proposedUpgradeFields
         );

slither.config.json

@@ -0,0 +1,4 @@
+{
+  "detectors_to_run": "arbitrary-send-erc20,array-by-reference,incorrect-shift,name-reused,rtlo,suicidal,uninitialized-storage,arbitrary-send-erc20-permit,controlled-array-length,controlled-delegatecall,delegatecall-loop,msg-value-loop,reentrancy-eth,unchecked-transfer,weak-prng,domain-separator-collision,erc20-interface,erc721-interface,locked-ether,mapping-deletion,shadowing-abstract,tautology,write-after-write,boolean-cst,reentrancy-no-eth,reused-constructor,tx-origin,unchecked-lowlevel,unchecked-send,variable-scope,void-cst,events-access,events-maths,incorrect-unary,boolean-equal,deprecated-standards,erc20-indexed,function-init-state,pragma,reentrancy-unlimited-gas,immutable-states,var-read-using-this",
+  "filter_paths": "(tests/|node_modules/|contracts/proto/)"
+}