Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: add slither job #431

Closed
wants to merge 10 commits into from
Closed

ci: add slither job #431

wants to merge 10 commits into from

Conversation

scorpion9979
Copy link
Contributor

Addresses #418.

ci: add more descriptive step names to slither job
ci: set slither target to "src" directory
ci: set solidity version for slither compilation to 0.8.19
ci: ignore "solc-version" check from slither job
@scorpion9979
Copy link
Contributor Author

@PaulRBerg, I have added a Slither job to ci.yml with the following steps:

  1. "Check out the repo": It checks out the repository, including submodules, using the "actions/checkout@v3" action.
  2. "Run Slither analysis": This step uses the "crytic/slither-action@v0.3.0" action to run Slither on the Solidity source code in the "src/" directory. It configures Slither to ignore naming-convention and solc-version checks, and to use Solidity compiler version 0.8.19. The analysis results are saved in the SARIF (Static Analysis Results Interchange Format) file named "results.sarif". The step does not fail the job if Slither finds any issues.
    Note: The target is set to "src/" instead of the default root directory of the project. Consequently, solc is used to compile the contracts instead of the default compilation framework for the root directory (i.e., Foundry).
  3. "Upload SARIF file to GitHub code scanning": This step uploads the "results.sarif" file to GitHub code scanning using the "github/codeql-action/upload-sarif@v2" action. This makes the analysis results available in the repository's "Security" tab and helps track potential vulnerabilities and code quality issues.
  4. "Add summary": This step adds a summary of the Slither analysis results to the GitHub Actions step summary, indicating that the results have been uploaded to GitHub code scanning.

I opted to compile the files in the target directory using solc instead of the default Foundry compilation framework, as there appeared to be issues with the Foundry configuration that are causing the Slither action to fail with the error: slither: exited with status 255; aborting.

Currently the Slither job is still failing, but this is due to the repo being private and thus requiring Advanced Security to be enabled for the repo to use code scanning.

@scorpion9979
Copy link
Contributor Author

Update: I have updated the CI script to use the configuration in "slither.config.json" to run the Slither job.

@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Apr 13, 2023

Update: After further investigation, I believe I have pinpointed the issue preventing the use of the local Foundry compilation framework in the Slither job. It seems that the filter_paths field in the Slither configuration is not working as expected.
When running slither ./ --config-file slither.config.json locally, the process fails with the same AssertionError encountered in CI when running the Slither job in CI under the same conditions. However, when the test/ directory is removed before executing the same command, even though it's already filtered out by the filter_paths field, the process successfully completes and generates the Slither report.

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @scorpion9979. The PR itself looks great, but the fact that GitHub Code Scanning is available only the Advanced paid plans is a dealbreaker. We will have to wait until we open-source this repository before we merge this PR :(

I opted to compile the files in the target directory using solc instead of the default Foundry compilation framework, as there appeared to be issues with the Foundry configuration

That's strange; the Slither action works in PRBProxy without having to target src.

https://github.com/PaulRBerg/prb-proxy/blob/71d09da34f9c97d08b145825e0109c17a60c4f9a/.github/workflows/ci.yml#L122-L127

I have pinpointed the issue preventing the use of the local Foundry compilation framework in the Slither job; It seems that the filter_paths field in the Slither configuration is not working as expected

Should we notify the Slither maintainers about this issue?

UPDATE: as per my comment here, I now realize that I bumped into the same issue when I have first run Slither in this repo. The problem may be due to Sablier using modern Solidity features, e.g. user-defined value types.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Member

Ah, wait .. there's another reason why we have to postpone the merge of this PR. Slither does not yet support user-defined operators, but PRBMath V4 (implemented in #432) provides operators. I tried to run Slither locally but got this error:

KeyError: 'function'
ERROR:root:Error in src/SablierV2LockupLinear.sol
ERROR:root:Traceback (most recent call last):

See the full terminal output here:

https://app.warp.dev/block/nkfChxLnBlSQ2pFWZAT0fO

ci: fix double quotes in slither job
ci: improve wording in slither summary step name
@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Apr 14, 2023

@PaulRBerg ,

Should we notify the Slither maintainers about this issue?

UPDATE: as per my comment #243 (comment), I now realize that I bumped into the same issue when I have first run Slither in this repo. The problem may be due to Sablier using modern Solidity features, e.g. user-defined value types.

After reading more about filter-paths in the Slither documentation, I started to think that its behavior might be in line with the maintainers' intentions, but easily misinterpreted by developers utilizing Slither. As per the Slither documentation:

--filter-paths path1 will exclude all the results that are only related to path1. The path specified can be a path directory or a filename.

This could suggest that the specified path will still be accessed by Slither, but any results from that path will be excluded from the final output. This explanation is in line with the behavior where the command slither . --filter-paths "test" fails, whereas the command slither . succeeds after removing the test/ directory. Maybe we we should notify the maintainers to clarify this in the documentation if this is indeed a case of miscommunication.

Ah, wait .. there's another reason why we have to postpone the merge of this PR. Slither does not yet support user-defined operators, but PRBMath V4 (implemented in #432) provides operators. I tried to run Slither locally but got this error:

KeyError: 'function'
ERROR:root:Error in src/SablierV2LockupLinear.sol
ERROR:root:Traceback (most recent call last):

See the full terminal output here:

https://app.warp.dev/block/nkfChxLnBlSQ2pFWZAT0fO

What version of Slither are you running? I tried running the same command locally and it worked fine.

Version:
Slither 0.9.3

Output:

$ slither src/SablierV2LockupLinear.sol

Compilation warnings/errors on src/SablierV2LockupLinear.sol:
Warning: Contract code size is 35175 bytes and exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on Mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.
  --> src/SablierV2LockupLinear.sol:41:1:
   |
41 | contract SablierV2LockupLinear is
   | ^ (Relevant source part starts here and spans across multiple lines).


INFO:Detectors:
SablierV2FlashLoan.flashLoan(IERC3156FlashBorrower,address,uint256,bytes) (src/abstracts/SablierV2FlashLoan.sol#109-183) uses arbitrary from in transferFrom: IERC20(asset).safeTransferFrom(address(receiver),address(this),returnAmount) (src/abstracts/SablierV2FlashLoan.sol#169)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#arbitrary-from-in-transferfrom
INFO:Detectors:
SablierV2LockupLinear._cancel(uint256) (src/SablierV2LockupLinear.sol#397-455) uses a dangerous strict equality:
        - senderAmount == 0 (src/SablierV2LockupLinear.sol#409)
SablierV2LockupLinear._cancel(uint256) (src/SablierV2LockupLinear.sol#397-455) uses a dangerous strict equality:
        - msg.sender == sender (src/SablierV2LockupLinear.sol#431)
SablierV2LockupLinear._isCallerStreamSender(uint256) (src/SablierV2LockupLinear.sol#309-311) uses a dangerous strict equality:
        - result = msg.sender == _streams[streamId].sender (src/SablierV2LockupLinear.sol#310)
SablierV2LockupLinear._streamedAmountOf(uint256) (src/SablierV2LockupLinear.sol#319-375) uses a dangerous strict equality:
        - status == Lockup.Status.DEPLETED (src/SablierV2LockupLinear.sol#324)
SablierV2LockupLinear._streamedAmountOf(uint256) (src/SablierV2LockupLinear.sol#319-375) uses a dangerous strict equality:
        - status == Lockup.Status.CANCELED (src/SablierV2LockupLinear.sol#328)
SablierV2LockupLinear.isSettled(uint256) (src/SablierV2LockupLinear.sol#195-201) uses a dangerous strict equality:
        - result = _streams[streamId].amounts.deposited == _streamedAmountOf(streamId) (src/SablierV2LockupLinear.sol#197)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities
INFO:Detectors:
SablierV2LockupLinear.createWithDurations(LockupLinear.CreateWithDurations).range (src/SablierV2LockupLinear.sol#260) is a local variable never initialized
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables
INFO:Detectors:
Adminable.transferAdmin(address).newAdmin (src/abstracts/Adminable.sol#34) lacks a zero-check on :
                - admin = newAdmin (src/abstracts/Adminable.sol#36)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation
INFO:Detectors:
SablierV2LockupLinear._cancel(uint256) (src/SablierV2LockupLinear.sol#397-455) has external calls inside a loop: ISablierV2LockupRecipient(recipient).onStreamCanceled(streamId,senderAmount,recipientAmount) (src/SablierV2LockupLinear.sol#433-437)
SablierV2LockupLinear._cancel(uint256) (src/SablierV2LockupLinear.sol#397-455) has external calls inside a loop: ISablierV2LockupSender(sender).onStreamCanceled(streamId,senderAmount,recipientAmount) (src/SablierV2LockupLinear.sol#445-449)
SablierV2LockupLinear._withdraw(uint256,address,uint128) (src/SablierV2LockupLinear.sol#546-586) has external calls inside a loop: ISablierV2LockupRecipient(recipient).onStreamWithdrawn(streamId,msg.sender,to,amount) (src/SablierV2LockupLinear.sol#576-581)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#calls-inside-a-loop
INFO:Detectors:
Reentrancy in SablierV2FlashLoan.flashLoan(IERC3156FlashBorrower,address,uint256,bytes) (src/abstracts/SablierV2FlashLoan.sol#109-183):
        External calls:
        - IERC20(asset).safeTransfer(address(receiver),amount) (src/abstracts/SablierV2FlashLoan.sol#145)
        - response = receiver.onFlashLoan(msg.sender,asset,amount,fee,data) (src/abstracts/SablierV2FlashLoan.sol#148-149)
        State variables written after the call(s):
        - protocolRevenues[IERC20(asset)] += uint128(fee) (src/abstracts/SablierV2FlashLoan.sol#162)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2
INFO:Detectors:
SablierV2LockupLinear (src/SablierV2LockupLinear.sol#41-587) does not implement functions:
        - ISablierV2Base.protocolRevenues(IERC20) (src/interfaces/ISablierV2Base.sol#47)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unimplemented-functions
INFO:Slither:src/SablierV2LockupLinear.sol analyzed (34 contracts with 81 detectors), 14 result(s) found

@PaulRBerg PaulRBerg closed this May 18, 2023
@PaulRBerg PaulRBerg deleted the ci/slither branch May 18, 2023 11:11
@PaulRBerg PaulRBerg restored the ci/slither branch May 18, 2023 11:12
@PaulRBerg PaulRBerg reopened this May 18, 2023
@PaulRBerg PaulRBerg force-pushed the main branch 5 times, most recently from 32fed59 to 4aafb70 Compare May 28, 2023 18:40
@PaulRBerg
Copy link
Member

FYI @scorpion9979, we've just made the repo public now - if you ever find a moment to rebase from main and re-configure Slither, we'd love to give this PR another shot!

However, it's worth noting that Slither still doesn't work with user-defined operators:

crytic/slither#1989

So I'm unsure if we can make Slither work with V2 Core, but it's worth trying!

@scorpion9979
Copy link
Contributor Author

Thanks, @PaulRBerg. I'll look into this and get back to you ASAP.

@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Jun 30, 2023

@PaulRBerg During my investigation of the problem, I was able to find a tiny bug in the Slither config that was causing the Slither CI job to fail at the solc remapping and compilation step. After addressing the bug, I was able to confirm that the job is now failing due to having no support for the user-defined operators used in prb-math.

Local Run

Command:

slither ./src --config-file slither.config.json

Version:

Slither 0.9.5

Output:

c57090fa-3d76-4964-aeb0-098105cd6485

@PaulRBerg
Copy link
Member

Thank you for your continued involvement in this PR, @scorpion9979! And sorry to hear that we can't merge this due to Slither not working with user-defined operators 😞

I will keep this PR open but I will mark it as backlog.

Local run
Output:

Thanks for sharing that.

Random pro tip: have you tried Warp? Their permalinks feature is pretty cool. The perfect solution for sharing terminal outputs as you did here.

@scorpion9979
Copy link
Contributor Author

Thanks, @PaulRBerg. Hopefully user-defined operators will be supported in time.

Re Warp: I've looked into using it, but unfortunately it's not available on Linux which is what I personally use.

@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Oct 20, 2023

@PaulRBerg Looks like Slither v0.10.0 fixes the crash on prb-math now, as mentioned in crytic/slither#1989 (comment). I've gone ahead and tested the repo locally with Slither v0.10.0 and I can confirm that it works as shown below. I've also gone ahead and merged the main branch into ci/slither in PR #713.

Local Run

Command:

slither ./src --config-file slither.config.json

Version:

Slither 0.10.0

Output:

image

@PaulRBerg
Copy link
Member

Very cool, thanks for the update @scorpion9979. Could you please fix the git conflicts?

And @andreivladbrg, could you please assist Ahmed by reviewing this PR?

@scorpion9979
Copy link
Contributor Author

@andreivladbrg PR #713 fixes the merge conflicts.

* docs: delete wikis

* test: polish precompiles tests

build: add branch for solady in ".gitmodules"

* docs: polish deployments section

* docs: polish security program

* docs: polish license section

* chore: remove Codecov token

* refactor: update gas snapshot

* build: change version to 1.0.0-rc.0

chore: update "repository" in "package.json"

* refactor: stringify create2 salts

* refactor: rename deployer to broadcaster

feat: use $ETH_FROM as broadcaster

* chore: fix rpc url for gnosis chain

* docs: refine license section

* test: update createSelectFork

* chore: use solhint-community

build: add "solhint-community" node.js dep
build: remove "solhint" node.js

* build: bump solady

* build: release v1.0.0

* test: remove duplicate bounds

* docs: mention audits in the bug bounty

* chore: say token streaming

* test: rename "basic" to "concrete"

* test: order imports correctly

* perf: use vars.sablierAddress

Closes #607

* perf: define "vars.asset" as "address"

Closes #608

* fix: data URI scheme

test: update hard-coded values
test: use LibString to remove data URI prefix

Closes #616

* test: update Precompiles bytecodes

* docs: roll v1.0.1

docs: add CHANGELOG

* docs: update recommended install branch

* chore: add missing modifier in test_SetFlashFee test

* Add Contribution info in README.md

* test: convert 'flashFee' integration tests to unit tests

* test: convert 'setFlashFee' integration tests to unit tests

* test: add 'Comptroller' setup for concrete unit test

* test: update 'flashFee' unit-test to inherit from Comptroller

* test: update 'setFlashFee' unit-test to inherit from Comptroller

* style: run prettier on README

* test: deploy correctly precompiled comptroller

* feat: deploy core 2

* test: change default profile value

* Fix error when importing @prb/math packages when installed via npm (#648)

* fix: address #646

* chore: update remapping in slither config

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* test: fix string parameter

* docs: roll v1.0.2

* docs: update recommended remapping

* docs: add "Architecture" in README

closes #551

* perf: dry "isCold" and "isWarm"

Closes #610

* docs: fix nitpicks

Closes #611

* test: use "given" keyword in branching trees (#656)

* refactor: using Given keyword in branching Trees (#641)

* Using Given keyword on the branching trees

(cherry picked from commit d365705)

* test: use "it" for test nodes

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

test: rename to "givenCallerAdmin"

test: use given keyword in the integration branching trees

test: use given keyword for modifiers for fuzz tests

test: use given keyword in test contracts and brancing trees

test: refactor test function names to use Given and When

test: refactor test function names to use Given and When

refactor: reverting in dev comments

refactor: rename test function names to cleaner form

test: polish branching trees

test: improve "mapSymbol" tree
test: improve "safeAssetDecimals" tree
test: improve "safeAssetSymbol" tree

* refactor: givenWithdrawableAmountNotZero modifier name

* test: use "when" for time-related branches

* test: address feedback in tree wording branch

* test: polish branching trees

* fix: remove duplicated branch in createWithMilestones.tree

---------

Co-authored-by: Shub <shubhamy2015@gmail.com>
Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>
Co-authored-by: Alexander González <alexfertel97@gmail.com>

* docs: mention audits

* docs: add Discord badge

* test: update comment

* fix: fix incorrect spec in isStream.tree (#677)

* test: use "given" instead of "when" (#678)

* test: use "given" instead of "when"

* test: correct "given" branches

---------

Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>

* ci: cache testing contracts (#681)

* Fix Github workflow build cache

* ci: add "out" dir in github cache

* Update .github/workflows/ci.yml

* Update .github/workflows/ci.yml

* Update .github/workflows/ci-deep.yml

---------

Co-authored-by: Lumyo <lumyo@proton.me>
Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* feat: add transferrable bool field for stream NFT

* feat: override _beforeTokenTransfer for Stream NFT and test cases

* test: Add test for NFT transfer functionality

* test: add more testing and resolve #669

* perf: dry "_beforeTokenTransfer"

refactor: remove "canStreamTransfer"
refactor: remove double "r" in "transferrable"
refactor: simplify implementation
test: allow non-transferable streams in invariant tests
test: dry tests for "isTransferable"
test: improve function and test names

* test: add a new branch to burn.tree

* test: polish burn tests

* test: update precompiles

* feat: implement _afterTokenTransfer to emit an event

* test: expect MetadataUpdate event to be emitted

* test: update Precompiles bytecode

* test: transferFrom function

* chore: add commented parameters in ERC721 hooks

* docs: improve writing in NatSpec

test: fix typos

* build: upgrade solidity version to 0.8.21 (#688)

* build: upgrade solidity version to 0.8.21

* build: bump the pragma back to >=0.8.19

* build: show unproved and unsupported SMTChecker

* refactor: update gas snapshot

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* refactor: change the streamed amount to total amount in NFT SVG (#692)

* refactor: change the streamed card from NFT SVG with total

build: update shell scripts accordingly
test: update tests accordingly

* chore: say "total amount" in comments for total card

* refactor: change the "Total" from NFT SVG with "Amount"

build: update shell script accordingly
test: update tests accordingly

* chore: fix typo in shell script

* refactor: dry-fy withdraw function

refactor: dry-fy renounce function

* build: remove OZ from peer dependencies

* docs: roll 1.1.0 (#693)

* docs: roll 1.1.0

* docs: update changelog

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* chore: fix typo in DataTypes

* docs: correct github hyperlink

docs: order change logs chronologically

* docs: update changelog

* build: switch to "solhint"

chore: disable Solhint rules

* refactor: capitalize immutable variables  (#700)

* refactor: capitalize immutable variables in NoDelegateCall

build: remove "immutable-vars-naming" from solhint file

* test: make asset and holder immutable in fork tests

test: capitalize constans in fork tests

* docs: polish background description

* chore: remove "cbor_metadata"

* perf: optimize withdraw function

* Remove the ability to cancel for recipients (#710)

* feat: remove ability to cancel for recipient

feat: remove sender's hook
test: update tests accordingly

* feat: emit asset in cancel and withdraw events

refactor: remove recipient from cancel event
test: update tests accordingly
test: update precompiles bytecode

* chore: update gas snapshot

* test: update Precompiles bytecode

* docs: include recipient disable to cancel in changelog

* refactor: add "recipient" in cancel event

docs: refine explanatory comments
refactor: reorder parameters in events

* test: update Precompiles bytecode

* chore: add explantory comment

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* test: add DeployOptimized utils contracts (#712)

* test: add DeployOptimized utils contracts

test: move and rename functions from Base_Test

* docs: fix typo in comments

* test: rename deploy optimized functions

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>

* style: fix Prettier formatting issue

---------

Co-authored-by: Paul Razvan Berg <paul.razvan.berg@gmail.com>
Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>
Co-authored-by: Anton Cheng <antonassocareer@gmail.com>
Co-authored-by: Prince Allwin <127643894+worksofallwin@users.noreply.github.com>
Co-authored-by: PraneshASP <praneshas2000@gmail.com>
Co-authored-by: Andrei Vlad Birgaoanu <99738872+andreivladbrg@users.noreply.github.com>
Co-authored-by: Shub <shubhamy2015@gmail.com>
Co-authored-by: Alexander González <alexfertel97@gmail.com>
Co-authored-by: Lumyo <lumyo@proton.me>
@andreivladbrg
Copy link
Member

@andreivladbrg PR #713 fixes the merge conflicts.

it seems it doesn't, there have been too many commits since this PR was created.

I think it would be better to start a new PR starting from main, and include this change.

Can you work on this @scorpion9979 ?

@scorpion9979 scorpion9979 mentioned this pull request Oct 20, 2023
@scorpion9979
Copy link
Contributor Author

@andreivladbrg Done here: #714

@andreivladbrg
Copy link
Member

Closing in favor of #714

@PaulRBerg PaulRBerg deleted the ci/slither branch January 1, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants