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

Add slither for l1 contracts #280

Merged
merged 32 commits into from
Mar 26, 2024
Merged

Add slither for l1 contracts #280

merged 32 commits into from
Mar 26, 2024

Conversation

dnkolegov
Copy link
Collaborator

@dnkolegov dnkolegov commented Mar 22, 2024

What ❔

This PR enables the slither static analyzer for L1 contracts and also resolves the issues it has found.
It is configured to catch high- and medium-severity potential issues.
It doesn't check Verifier.sol contract.

The PR also documents how to deal with findings.

Why ❔

To find "low-hanging fruits" bugs and known vulnerabilities.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.

@dnkolegov dnkolegov marked this pull request as ready for review March 25, 2024 18:43
@dnkolegov
Copy link
Collaborator Author

@StanislavBreadless as we discussed with @vladbochok could you please review the following findings

MailboxFacet._deriveL2GasPrice(uint256,uint256) (contracts/state-transition/chain-deps/facets/Mailbox.sol#163-182) performs a multiplication on the result of a division:
	- l1GasPriceConverted = (_l1GasPrice * s.baseTokenGasPriceMultiplierNominator) / s.baseTokenGasPriceMultiplierDenominator (contracts/state-transition/chain-deps/facets/Mailbox.sol#[16](https://github.com/matter-labs/era-contracts/actions/runs/8425289708/job/23071021468?pr=280#step:9:17)6-167)
	- batchOverheadBaseToken = uint256(feeParams.batchOverheadL1Gas) * l1GasPriceConverted (contracts/state-transition/chain-deps/facets/Mailbox.sol#173)
MailboxFacet._deriveL2GasPrice(uint256,uint256) (contracts/state-transition/chain-deps/facets/Mailbox.sol#163-182) performs a multiplication on the result of a division:
	- l1GasPriceConverted = (_l1GasPrice * s.baseTokenGasPriceMultiplierNominator) / s.baseTokenGasPriceMultiplierDenominator (contracts/state-transition/chain-deps/facets/Mailbox.sol#166-167)
	- pubdataPriceBaseToken = L1_GAS_PER_PUBDATA_BYTE * l1GasPriceConverted (contracts/state-transition/chain-deps/facets/Mailbox.sol#[17](https://github.com/matter-labs/era-contracts/actions/runs/8425289708/job/23071021468?pr=280#step:9:18)0)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

and let us know how we should proceed. Should it be fixed or ignored? If fixed, then could you add a fix into this PR?
Thank you.

.github/workflows/slither.yaml Outdated Show resolved Hide resolved
l1-contracts/README.md Outdated Show resolved Hide resolved
l1-contracts/foundry.toml Outdated Show resolved Hide resolved
@StanislavBreadless
Copy link
Collaborator

@dnkolegov we can ignore it. If we used what slither proposed, the calculations would indeed be slightly more precise, but IMHO the way it is now is a bit easier to read & reuse (i.e. whenever we need l1 gas price we can use l1GasPriceConverted)

Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
@dnkolegov dnkolegov merged commit e1fe9d9 into dev Mar 26, 2024
19 checks passed
@dnkolegov dnkolegov deleted the denis/dev_slither branch March 26, 2024 14:00
matzayonc pushed a commit to neotheprogramist/era-contracts that referenced this pull request Mar 27, 2024
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.

2 participants