ci: added initial slither to ci

[0xcadams]

What does this pull request do? Explain your changes. (required) Adds Slither to CI with Github Code Scanning integration.

Specific updates (required) CI updates + security analysis comments integration w/ GH Slither has certain noisy configs ignored (for now)

How did you test each of these updates (required) See below

Does this pull request close any open issues? Fixes https://github.com/livepeer/protocol/issues/567

Checklist:

• [x] README and other documentation updated
• [x] All tests using yarn test pass

[RiccardoBiosas]

Changes look good but what could be done with the alerts which fail the CI. Would dismissing an alert once cause it to re-emerge when a new PR is made or would it save it's response?

IMHO I don't think it's desirable to force a CI action to fail on the basis of the result of Slither since static analysis is known to give a fair amount of false positives. According to github docs, if you dismiss an alert it should be for good, however github will document the dismissed alerts and the reasons why they have been dismissed

[kautukkundan]

According to github docs, if you dismiss an alert it should be for good

Thanks for sharing this, I had been looking in the wrong docs

IMHO I don't think it's desirable to force a CI action to fail on the basis of the result of Slither since static analysis is known to give a fair amount of false positives.

I agree with this, adding continue-on-error: true will allow-failures. The CI would pass and we would still have result for static code analysis. This should should be suitable for our case.

[0xcadams]

adding continue-on-error: true will allow-failures.

This actually already exists on the Slither step - the failure is a GitHub code scanning failure, so we'd have to manually dismiss the security results we'd like to ignore/create issues on the ones we'd like to address.

IMHO I don't think it's desirable to force a CI action to fail on the basis of the result of Slither since static analysis is known to give a fair amount of false positives.

Yeah, the false positives can be a bit annoying. I think if we remove the failures altogether, we will start to ignore the results of slither. I'd advocate for a process of adding comments to manually opt out of Slither false positives, or manually dismissing the security results as "false positive". Otherwise this CI step will get ignored.

[kautukkundan]

This actually already exists on the Slither step - the failure is a GitHub code scanning failure, so we'd have to manually dismiss the security results we'd like to ignore/create issues on the ones we'd like to address.

Ah yes sorry, just found out that it doesn't work as I expected.

manually dismissing the security results as "false positive"

I also think this would be an easy way forward. Once we address these alerts they will go away and for future contract updates I think it's nice to have explicit alerts even if they are FPs. The reviewer could address (or dismiss) these before merging. WDYT @RiccardoBiosas.

[RiccardoBiosas]

I also think this would be an easy way forward. Once we address these alerts they will go away and for future contract updates I think it's nice to have explicit alerts even if they are FPs. The reviewer could address (or dismiss) these before merging. WDYT @RiccardoBiosas.

Isn't it what the PR is already doing? The only issues that are dismissed by default are informational

[kautukkundan]

Isn't it what the PR is already doing?

Yeah, I meant that it's fine only, the way it is, addressing your comment ⬇️

IMHO I don't think it's desirable to force a CI action to fail on the basis of the result of Slither

[0xcadams]

There are actual errors here we need to address - for instance, the unchecked transfer issues are something we should not dismiss.

Unchecked transfer
[BondingManager.bondForWithHint(uint256,address,address,address,address,address,address)](https://github.com/livepeer/protocol/pull/570/contracts/bonding/BondingManager.sol#L475-L549) ignores return value by [livepeerToken().transferFrom(msg.sender,address(minter()),_amount)](https://github.com/livepeer/protocol/pull/570/contracts/bonding/BondingManager.sol#L545)

I can lump those changes into this PR. As far as the others are concerned, we may want to jump on a 10 min call to make sure we're okay with dismissing them permanently. WDYT @RiccardoBiosas @kautukkundan @yondonfu @red-0ne

[0xcadams]

• can you also run slither with the slither-check-upgradeability plugin?

I tried using the upgradeable plugin, but it depends on following the OZ standards for upgradeable contracts:

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

• I think it'd be best to have a separate github action for Slither rather than lumping everything together with the other tests
• [question] seems like codeQL action v1 will be EOL in december, so is worth to switch to v2 right away?

I will address these!

[RiccardoBiosas]

I can lump those changes into this PR. As far as the others are concerned, we may want to jump on a 10 min call to make sure we're okay with dismissing them permanently. WDYT @RiccardoBiosas @kautukkundan @yondonfu @red-0ne

yes good idea, i think some of them should be added to the backlog anyway

[0xcadams]

@hjpotter92 resolved those issues @RiccardoBiosas @kautukkundan @yondonfu @red-0ne SARIF upload has been removed for now, and Analyze CI will fail currently until all Slither code quality issues are addressed. Let me know if you'd like to see any changes to this.