From ce4437c6d527827a930d940d07a548efa409d95a Mon Sep 17 00:00:00 2001 From: Matjaz Verbole Date: Mon, 24 Jun 2024 09:43:51 +0200 Subject: [PATCH] Add automated toolings for testing contracts (#151) * Add Slither support in GH actions * Add slither-mutate support for local execution * Add readme for test directory * Fix rebasing with development * Add Echidna support for local execution * Fix spelling typo --- .github/workflows/pr.yaml | 34 +++++++------ .gitignore | 9 ++++ .gitmodules | 3 ++ foundry.toml | 1 - lib/properties | 1 + test/L1/L1LiskToken.t.sol | 4 +- test/L1/L1VestingWallet.t.sol | 2 +- test/L2/L2Claim.t.sol | 2 +- test/L2/L2LiskToken.t.sol | 6 +-- test/L2/L2LockingPosition.t.sol | 2 +- test/L2/L2Reward.t.sol | 2 +- test/L2/L2VestingWallet.t.sol | 2 +- test/L2/L2VotingPower.t.sol | 4 +- test/L2/paused/L2GovernorPaused.t.sol | 2 +- test/L2/paused/L2VotingPowerPaused.t.sol | 2 +- test/README.md | 46 +++++++++++++++++ test/fuzzing/L2Reward.echidna.sol | 64 ++++++++++++++++++++++++ test/scripts/runEchidna.sh | 19 +++++++ test/scripts/runSlither.sh | 20 ++++++++ test/scripts/runSlitherMutate.sh | 20 ++++++++ 20 files changed, 216 insertions(+), 29 deletions(-) create mode 160000 lib/properties create mode 100644 test/README.md create mode 100644 test/fuzzing/L2Reward.echidna.sol create mode 100644 test/scripts/runEchidna.sh create mode 100755 test/scripts/runSlither.sh create mode 100755 test/scripts/runSlitherMutate.sh diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index d7fa4297..2d7b330e 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -6,6 +6,7 @@ on: permissions: contents: read + security-events: write env: FOUNDRY_PROFILE: ci @@ -29,21 +30,10 @@ jobs: with: submodules: recursive - #- name: Install Foundry - # uses: foundry-rs/foundry-toolchain@v1 - # with: - # version: nightly - - - uses: actions/checkout@v4 + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 with: - repository: foundry-rs/foundry - path: ./foundry - submodules: recursive - ref: 5ac78a9cd4b94dc53d1fe5e0f42372b28b5a7559 - - - name: Install Forge - run: | - cargo install --path ./foundry/crates/forge --profile local --force --locked + version: nightly - name: Check formatting if: ${{ contains(matrix.system.os, 'windows') == false }} @@ -61,3 +51,19 @@ jobs: run: | forge test -vvv id: test + + - name: Run Slither + if: ${{ contains(matrix.system.os, 'ubuntu') == true }} + uses: crytic/slither-action@v0.4.0 + with: + solc-version: 0.8.23 + sarif: results.sarif + fail-on: none + slither-args: --exclude-dependencies --exclude-low --exclude-informational --filter-paths "Ed25519.sol" + id: slither + + - name: Upload SARIF file + if: ${{ contains(matrix.system.os, 'ubuntu') == true }} + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: ${{ steps.slither.outputs.sarif }} diff --git a/.gitignore b/.gitignore index bc674785..7acc010e 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,15 @@ broadcast/ # Slither files slither-results.json +slitherResults.md # Devnet deployment files deployment/devnet/ + +# Slither-mutation files +mutation_campaign + +# Echidna files +crytic-export/combined_solc.json +test/fuzzing/coverage/ +test/fuzzing/covered.* diff --git a/.gitmodules b/.gitmodules index f27e450e..feb437a3 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,6 +7,9 @@ [submodule "lib/openzeppelin-contracts-upgradeable"] path = lib/openzeppelin-contracts-upgradeable url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable +[submodule "lib/properties"] + path = lib/properties + url = https://github.com/crytic/properties [submodule "lib/openzeppelin-foundry-upgrades"] path = lib/openzeppelin-foundry-upgrades url = https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades diff --git a/foundry.toml b/foundry.toml index e4e58aa3..1bf6088c 100644 --- a/foundry.toml +++ b/foundry.toml @@ -20,7 +20,6 @@ remappings = [ 'solidity-stringutils/=lib/openzeppelin-foundry-upgrades/lib/solidity-stringutils/', ] deny_warnings = true - ffi = true ast = true build_info = true diff --git a/lib/properties b/lib/properties new file mode 160000 index 00000000..bb1b7854 --- /dev/null +++ b/lib/properties @@ -0,0 +1 @@ +Subproject commit bb1b78542b3f38e4ae56cf87389cd3ea94387f48 diff --git a/test/L1/L1LiskToken.t.sol b/test/L1/L1LiskToken.t.sol index 681f33f8..5f0ae1e7 100644 --- a/test/L1/L1LiskToken.t.sol +++ b/test/L1/L1LiskToken.t.sol @@ -26,7 +26,7 @@ contract L1LiskTokenTest is Test { l1LiskToken = new L1LiskToken(); } - function test_Initialize() public { + function test_Initialize() public view { assertEq(l1LiskToken.name(), NAME); assertEq(l1LiskToken.symbol(), SYMBOL); assertEq(l1LiskToken.totalSupply(), TOTAL_SUPPLY); @@ -199,7 +199,7 @@ contract L1LiskTokenTest is Test { assertEq(l1LiskToken.pendingOwner(), alice); } - function test_DefaultAdminRoleIsRoleAdminForBurnerRole() public { + function test_DefaultAdminRoleIsRoleAdminForBurnerRole() public view { assertEq(l1LiskToken.DEFAULT_ADMIN_ROLE(), l1LiskToken.getRoleAdmin(l1LiskToken.BURNER_ROLE())); } } diff --git a/test/L1/L1VestingWallet.t.sol b/test/L1/L1VestingWallet.t.sol index e51ee1ee..782f4c17 100644 --- a/test/L1/L1VestingWallet.t.sol +++ b/test/L1/L1VestingWallet.t.sol @@ -77,7 +77,7 @@ contract L1VestingWalletTest is Test { mockToken.transfer(address(l1VestingWallet), vestAmount); } - function test_Initialize() public { + function test_Initialize() public view { assertEq(l1VestingWallet.name(), name); assertEq(l1VestingWallet.start(), startTimestamp); assertEq(l1VestingWallet.duration(), durationSeconds); diff --git a/test/L2/L2Claim.t.sol b/test/L2/L2Claim.t.sol index 9fc9f6e2..7a0dc0a5 100644 --- a/test/L2/L2Claim.t.sol +++ b/test/L2/L2Claim.t.sol @@ -191,7 +191,7 @@ contract L2ClaimTest is Test { l2ClaimImplementation.initialize(address(lsk), bytes32(0), block.timestamp + RECOVER_PERIOD); } - function test_Version() public { + function test_Version() public view { assertEq(l2Claim.version(), "1.0.0"); } diff --git a/test/L2/L2LiskToken.t.sol b/test/L2/L2LiskToken.t.sol index b87acbcb..a84a561f 100644 --- a/test/L2/L2LiskToken.t.sol +++ b/test/L2/L2LiskToken.t.sol @@ -44,7 +44,7 @@ contract L2LiskTokenTest is Test { new L2LiskToken(address(0)); } - function test_Initialize() public { + function test_Initialize() public view { assertEq(l2LiskToken.name(), "Lisk"); assertEq(l2LiskToken.symbol(), "LSK"); assertEq(l2LiskToken.decimals(), 18); @@ -176,12 +176,12 @@ contract L2LiskTokenTest is Test { assertNotEq(address(l2LiskTokenSalted), l2LiskTokenAddressCalculated); } - function test_GetBridge() public { + function test_GetBridge() public view { assertEq(l2LiskToken.bridge(), bridge); assertEq(l2LiskToken.BRIDGE(), bridge); } - function test_GetRemoteToken() public { + function test_GetRemoteToken() public view { assertEq(l2LiskToken.remoteToken(), remoteToken); assertEq(l2LiskToken.REMOTE_TOKEN(), remoteToken); } diff --git a/test/L2/L2LockingPosition.t.sol b/test/L2/L2LockingPosition.t.sol index a8bf224f..b83351a1 100644 --- a/test/L2/L2LockingPosition.t.sol +++ b/test/L2/L2LockingPosition.t.sol @@ -517,7 +517,7 @@ contract L2LockingPositionTest is Test { l2LockingPosition.removeLockingPosition(positionId); } - function test_GetLockingPosition_PositionDoesNotExist() public { + function test_GetLockingPosition_PositionDoesNotExist() public view { IL2LockingPosition.LockingPosition memory position = l2LockingPosition.getLockingPosition(1); assertEq(position.creator, address(0)); assertEq(position.amount, 0); diff --git a/test/L2/L2Reward.t.sol b/test/L2/L2Reward.t.sol index 2d3b825b..451492e4 100644 --- a/test/L2/L2Reward.t.sol +++ b/test/L2/L2Reward.t.sol @@ -113,7 +113,7 @@ contract L2RewardTest is Test { l2Staking.addCreator(address(l2Reward)); } - function test_initialize() public { + function test_initialize() public view { assertEq(l2Reward.lastTrsDate(), deploymentDate); assertEq(l2Reward.OFFSET(), 150); assertEq(l2Reward.REWARD_DURATION(), 30); diff --git a/test/L2/L2VestingWallet.t.sol b/test/L2/L2VestingWallet.t.sol index 06defb95..a604a5c5 100644 --- a/test/L2/L2VestingWallet.t.sol +++ b/test/L2/L2VestingWallet.t.sol @@ -76,7 +76,7 @@ contract L2VestingWalletTest is Test { mockToken.transfer(address(l2VestingWallet), vestAmount); } - function test_Initialize() public { + function test_Initialize() public view { assertEq(l2VestingWallet.name(), name); assertEq(l2VestingWallet.start(), startTimestamp); assertEq(l2VestingWallet.duration(), durationSeconds); diff --git a/test/L2/L2VotingPower.t.sol b/test/L2/L2VotingPower.t.sol index 535614be..8239a63a 100644 --- a/test/L2/L2VotingPower.t.sol +++ b/test/L2/L2VotingPower.t.sol @@ -99,7 +99,7 @@ contract L2VotingPowerTest is Test { ); } - function test_Version() public { + function test_Version() public view { assertEq(l2VotingPower.version(), "1.0.0"); } @@ -329,7 +329,7 @@ contract L2VotingPowerTest is Test { assertEq(l2VotingPower.clock(), blockTimestamp + 1); } - function test_ClockMode() public { + function test_ClockMode() public view { assertEq(l2VotingPower.CLOCK_MODE(), "mode=timestamp"); } diff --git a/test/L2/paused/L2GovernorPaused.t.sol b/test/L2/paused/L2GovernorPaused.t.sol index 41cd0720..b15f9f43 100644 --- a/test/L2/paused/L2GovernorPaused.t.sol +++ b/test/L2/paused/L2GovernorPaused.t.sol @@ -32,7 +32,7 @@ contract L2GovernorPausedTest is Test, ERC1155Holder, ERC721Holder { TimelockController timelock; address initialOwner; - function assertInitParamsEq() internal { + function assertInitParamsEq() internal view { assertEq(l2Governor.name(), "Lisk Governor"); assertEq(l2Governor.votingDelay(), 0); assertEq(l2Governor.votingPeriod(), 604800); diff --git a/test/L2/paused/L2VotingPowerPaused.t.sol b/test/L2/paused/L2VotingPowerPaused.t.sol index 0fdccb88..c27670a0 100644 --- a/test/L2/paused/L2VotingPowerPaused.t.sol +++ b/test/L2/paused/L2VotingPowerPaused.t.sol @@ -33,7 +33,7 @@ contract L2VotingPowerPausedTest is Test { address lockingPositionContractAddress; - function assertInitParamsEq() internal { + function assertInitParamsEq() internal view { assertEq(l2VotingPower.lockingPositionAddress(), lockingPositionContractAddress); assertEq(l2VotingPower.name(), "Lisk Voting Power"); assertEq(l2VotingPower.symbol(), "vpLSK"); diff --git a/test/README.md b/test/README.md new file mode 100644 index 00000000..013911a1 --- /dev/null +++ b/test/README.md @@ -0,0 +1,46 @@ +# Testing Smart Contracts with Foundry and Other Frameworks + +This repository utilizes the Foundry and other frameworks for testing smart contracts. Within the `test` directory, you will find multiple script files designed to be executed locally. These scripts are integral for different testing and analysis tools: `Slither`, `slither-mutate`, and `Echidna`. Below is an overview of each script and the respective tools they utilize. + +## Overview of Scripts + +1. **Slither Script** + - **Script File:** `scripts/runSlither.sh` + - **Purpose:** This script runs the `Slither` tool, which is used for static analysis of smart contracts. + - **Details:** `Slither` helps in identifying potential vulnerabilities and issues in the smart contract code without executing it. It provides insights into security, correctness, and gas optimization. + +2. **Slither-Mutate Script** + - **Script File:** `scripts/runSlitherMutate.sh` + - **Purpose:** This script executes the `slither-mutate` tool, which extends `Slither`’s capabilities by introducing mutation testing. + - **Details:** `Slither-mutate` creates variations (mutants) of the smart contract code to check if the existing test cases can detect these mutations. It helps in assessing the effectiveness and coverage of the test suite. + +3. **Echidna Script** + - **Script File:** `scripts/runEchidna.sh` + - **Purpose:** This script runs the `Echidna` tool, a property-based fuzzer. + - **Details:** `Echidna`is used to find bugs by generating random inputs to test the smart contract properties. It helps in uncovering edge cases and ensuring that the contracts behave as expected under various conditions. + +## Tools Overview + +### Slither + +`Slither` is a static analysis framework specifically designed for smart contracts. It analyzes the contract code without executing it, providing valuable insights into: +- Security vulnerabilities +- Code correctness +- Optimization opportunities +- Compliance with best practices + +### Slither-Mutate + +`Slither-mutate` leverages the static analysis power of `Slither` and combines it with mutation testing. By creating and testing mutated versions of the contract, it evaluates: +- The robustness of the existing test cases +- The overall test coverage +- The ability of the tests to detect potential issues + +### Echidna + +`Echidna` is a property-based testing tool (fuzzer) for smart contracts. It works by: +- Generating random inputs to interact with the smart contract +- Checking predefined properties and invariants +- Identifying unexpected behavior or bugs + +To get familiar with `Echinida` tool, you can refer to the Secure Contracts website and follow [Echidna tutorial](https://secure-contracts.com/program-analysis/echidna/index.html). diff --git a/test/fuzzing/L2Reward.echidna.sol b/test/fuzzing/L2Reward.echidna.sol new file mode 100644 index 00000000..a9c3f39b --- /dev/null +++ b/test/fuzzing/L2Reward.echidna.sol @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity 0.8.23; + +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { PropertiesAsserts } from "properties/util/PropertiesHelper.sol"; +import { L2LiskToken } from "src/L2/L2LiskToken.sol"; +import { L2LockingPosition } from "src/L2/L2LockingPosition.sol"; +import { L2Staking } from "src/L2/L2Staking.sol"; + +interface iHevm { + function prank(address sender) external; +} + +contract L2RewardEchidnaTest is PropertiesAsserts { + L2Staking public l2Staking; + L2LockingPosition public l2LockingPosition; + + iHevm public hevm; + + constructor() { + hevm = iHevm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); + address bridge = address(0x1234567890123456789012345678901234567890); + address remoteToken = address(0xaBcDef1234567890123456789012345678901234); + + hevm.prank(msg.sender); + L2LiskToken l2LiskToken = new L2LiskToken(remoteToken); + l2LiskToken.initialize(bridge); + + L2Staking l2StakingImplementation = new L2Staking(); + l2Staking = L2Staking( + address( + new ERC1967Proxy( + address(l2StakingImplementation), + abi.encodeWithSelector(l2Staking.initialize.selector, address(l2LiskToken)) + ) + ) + ); + + L2LockingPosition l2LockingPositionImplementation = new L2LockingPosition(); + l2LockingPosition = L2LockingPosition( + address( + new ERC1967Proxy( + address(l2LockingPositionImplementation), + abi.encodeWithSelector(l2LockingPosition.initialize.selector, address(l2Staking)) + ) + ) + ); + } + + function durationIsNeverLargerThanMax(uint256 lockID) public { + lockID = PropertiesAsserts.clampBetween(lockID, 1, 1000); + (,, uint256 expDate, uint256 pausedDuration) = l2LockingPosition.lockingPositions(lockID); + uint256 maxDuration = l2Staking.MAX_LOCKING_DURATION(); + uint256 today = block.timestamp / 1 days; + PropertiesAsserts.assertWithMsg( + pausedDuration <= maxDuration, "The position paused duration is larger than max!" + ); + if (expDate > today) { + PropertiesAsserts.assertWithMsg( + expDate - today <= maxDuration, "The position unpaused duration is larger than max!" + ); + } + } +} diff --git a/test/scripts/runEchidna.sh b/test/scripts/runEchidna.sh new file mode 100644 index 00000000..7c928b80 --- /dev/null +++ b/test/scripts/runEchidna.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash + +echo "Instructing the shell to exit immediately if any command returns a non-zero exit status..." +set -e +echo "Done." + +echo "Navigating to the root directory of the project..." +cd ../../ +echo "Done." + +if [ -z "$1" ] +then + echo "Please provide the contract name as the first argument."$'\n'"Example: runEchidna.sh " + exit 1 +fi + +echo "Starting Echidna tool..." +echidna test/fuzzing --workers 5 --contract $1 --corpus-dir test/fuzzing --format text --test-mode assertion --test-limit 5000000 +echo "Done." diff --git a/test/scripts/runSlither.sh b/test/scripts/runSlither.sh new file mode 100755 index 00000000..8ebaca51 --- /dev/null +++ b/test/scripts/runSlither.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash + +echo "Instructing the shell to exit immediately if any command returns a non-zero exit status..." +set -e +echo "Done." + +echo "Navigating to the root directory of the project..." +cd ../../ +echo "Done." + +echo "Removing file slitherResults.md if it exists..." +if [ -f "slitherResults.md" ] +then + rm slitherResults.md +fi +echo "Done." + +echo "Starting Slither tool..." +slither . --exclude-dependencies --exclude-low --exclude-informational --filter-paths "Ed25519.sol" --checklist > slitherResults.md +echo "Done." diff --git a/test/scripts/runSlitherMutate.sh b/test/scripts/runSlitherMutate.sh new file mode 100755 index 00000000..f2421d01 --- /dev/null +++ b/test/scripts/runSlitherMutate.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash + +echo "Instructing the shell to exit immediately if any command returns a non-zero exit status..." +set -e +echo "Done." + +echo "Navigating to the root directory of the project..." +cd ../../ +echo "Done." + +echo "Removing directory mutation_campaign if it exists..." +if [ -d "mutation_campaign" ] +then + rm -rf mutation_campaign +fi +echo "Done." + +echo "Starting Slither Mutate Campaign..." +slither-mutate . --test-cmd='forge test' --test-dir='test' --ignore-dirs='script,lib,test,utils,cache,out,broadcast,deployment' +echo "Done."