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

Resolve warnings for Staking and DAO smart contracts found by Slither #79

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

matjazv
Copy link
Contributor

@matjazv matjazv commented Apr 15, 2024

What was the problem?

This PR resolves #69.

How was it solved?

  • All important Slither warnings were resolved.

How was it tested?

Unit tests were modified accordingly to all changes.

Additional Information

One Low level warning was not resolved:

  • uses timestamp for comparisons; Recommendation: Avoid relying on block.timestamp

One Informational level warning was not resolved:

  • compares to a boolean constant; Recommendation: Remove the equality to the boolean constant.

Contracts are calculating todays day based on block.timestamp and in my opinion it's safe to use block.timestamp.
This recommendation would be valid if we'll be using block.timestamp for calculation of randomness. In this case block proposer could influence on block.timestamp and took his advantage on others.

For informational warning, it advises to instead of using:
require(isLockingPositionNull(lock) == false, "L2Staking: locking position does not exist");
we should use:
require(!isLockingPositionNull(lock), "L2Staking: locking position does not exist");
which is in my opinion more error prone than the first version.

@matjazv matjazv self-assigned this Apr 15, 2024
@matjazv matjazv changed the title 69 slither warnings Resolve warnings for Staking and DAO smart contracts found by Slither Apr 15, 2024
src/interfaces/L1/iL1LiskToken.sol Outdated Show resolved Hide resolved
script/generateInterfaces.sh Outdated Show resolved Hide resolved
test/L2/L2VotingPower.t.sol Outdated Show resolved Hide resolved
@Phanco
Copy link
Member

Phanco commented Apr 16, 2024

Is it intentional that some interfaces starts with capital I some with i?

@matjazv
Copy link
Contributor Author

matjazv commented Apr 16, 2024

Is it intentional that some interfaces starts with capital I some with i?

Nice catch.
Only files which didn't exist before script was ran have capital I.
I've fixed the script and wrong file names.

Copy link
Member

@Phanco Phanco left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@matjazv matjazv merged commit b3448af into main Apr 16, 2024
3 checks passed
@matjazv matjazv deleted the 69-slither-warnings branch April 16, 2024 11:26
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.

Resolve warnings for all newly implemented smart contracts found by Slither
3 participants