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

fix: invariant tests #129

Merged
merged 12 commits into from
Jul 31, 2024
Merged

fix: invariant tests #129

merged 12 commits into from
Jul 31, 2024

Conversation

hexshire
Copy link
Member

@hexshire hexshire commented Jul 29, 2024

🤖 Linear

Closes OPT-183

@hexshire hexshire changed the base branch from dev to audit/spearbit July 29, 2024 19:49
@hexshire hexshire changed the title Fix/invariant tests fix: invariant tests Jul 30, 2024
Copy link
Contributor

@0xDiscotech 0xDiscotech left a comment

Choose a reason for hiding this comment

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

  • Missing invariant test of property 3. Is that on purpose or we're missing it?

  • On fuzz_BurnLockedUSDC, isn't missing a check on the catch? currently we're doing assert(false) but shouldn't we check that the amount is not burned?
    Also, what about checking that it can't be burned if the state is not deprecated? Maybe I'm wrong bc I'm not used to echidna, but aren't we testing only the happy path there?

  • A similar behaviour to the point mentioned above is present on fuzz_l1LinkedAdapterIncommingMessages and fuzz_l2LinkedAdapterIncommingMessages, where the catch clause is empty. Shouldn't we be testing that the message never suceeded andbecause of that the state didn't change?

  • A similar empty catch scenario exists on fuzz_onlyMigrateToNativeForOwnershipTransfer

Can't comment because they werent modified:

  • For consistency we should use 9_000_000 instead of 3_000_000 as gas limit when executing the deployments

Here is ensure:
image
Also check sometimes you used Insura and Insure again

Here remove the 9
image

test/invariants/fuzz/SetupOpUSDC.sol Show resolved Hide resolved
test/invariants/fuzz/SetupOpUSDC.sol Outdated Show resolved Hide resolved
@defi-wonderland defi-wonderland deleted a comment from linear bot Jul 30, 2024
Copy link

linear bot commented Jul 30, 2024

@hexshire
Copy link
Member Author

@0xDiscotech > * Missing invariant test of property 3. Is that on purpose or we're missing it?

"Assuming the adapter is the only minter the amount locked in L1 should always equal the amount minted on L2"

This is something that should be modify since now the total might be locked on l1 == totalSupply + blacklistedFunds

hexshire and others added 4 commits July 30, 2024 16:21
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
@@ -19,6 +19,7 @@
| USDC proxy admin and token ownership rights can only be transferred during the migration to native flow | High level | 17 | [X] | |
| Status should either be active, paused, upgrading or deprecated | Valid state | 18 | [X] | |
| All addresses precomputed in the factory match the deployed addresses / L1 nonce == L2 factory nonce | Variable transition | | depr | depr |
| Adapters can't be initialized twice | State transition | 19 | [] | |
Copy link
Member

Choose a reason for hiding this comment

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

nit> don't leave an empty box for a new prop (so we know it still need to be at least tried;)

@excaliborr excaliborr merged commit d443487 into audit/spearbit Jul 31, 2024
4 checks passed
@excaliborr excaliborr deleted the fix/invariant-tests branch July 31, 2024 17:54
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.

4 participants