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

Audit Fixes - System Changes - Code Refactoring - Mega Merge #17

Merged
merged 363 commits into from
Nov 13, 2024

Conversation

GalloDaSballo
Copy link
Collaborator

@GalloDaSballo GalloDaSballo commented Oct 10, 2024

Invariant Tests

https://getrecon.xyz/shares/478a5ff7-e63d-4075-bd9d-5dc5a51fe739

NOTE: CI/CD re-uses a 200 MLN tests run to massively speed up testing changes
Make sure to run a proper 200MLN+ run after all mitigations have been applied

1 BLN tests (after adding a canary for claimForInitiative:
https://getrecon.xyz/shares/3d6c3753-1ecb-4f6f-b1e4-7661679bf632

Mitigation Notes

Mega PR of all changes

This PR will not be merged until the mitigation review is done

The current WIP PR is: #16

This PR collects additional small changes and fixes

5.1 -

Fixed, see: #16

Fix: 5.2 - Try Catch that actually doesn't revert - #5

  • Fix known vector such as: selfdestruct, returnbomb, gasgrief and reverts
  • Discuss Gas to Hooks
  • Basic Tests
  • Invariant Testing on Governance to prevent any DOS | cc: @nican0r | High impact | Mid Prio
  • Invariant Testing Setup to test Initiatives to verify if they are safe to use | cc: @nican0r | Low Prio | High Impact

Fix: 5.3 - fix: counted ts fix - #18

  • Write tests to identify the bug
  • Write fuzz / invariant tests to ensure the bug doesn't happen again
    @nican0r need coverage here pls
    Missing Test here
    Can write a basic one:
    Given a User Voting contributing X to the total
    Removing 100% of votes should remove X off the total

5.4 - WIP - See: #16

5.5 - FIXED - #19

Fix: 5.6 - see 5.2

5.7 -> See 5.4

5.8 -> See 5.4

Fix: 5.9 - Curve Claim Flow + Tests + Griefing - #10

NOTE:
You will have to get the initiative approved by the CurveDAO

If the voting time is 7 days, then this should be fine
Even if the voting time is longer, as long as you get it approved within less than 14 days from deployment, the entirety of the system will work and no epoch will be skipped

It's worth doing a review / simulation post deployment, but the local tests do use the NG implementations which seem to be the latest

5.10 -> See 5.4

5.11

Awaiting @jltqy See: fix/voteCheck
May rewrite these

5.12

Ack


Fix: V4 Claim Grief Flow - #11
We have the followign doubts:

  • Does the hook work?
  • Does the vesting work
  • E2E Fork Test

Recommended steps:

  • Use official univ4 template
  • Test E2E on fork mainnet

Additional Bug Fixes

Voting Power Desynch (Med / High)
Fixed by enforcing a reset
#49

Minor

Unchecked ETH Transfer - #7

  • Adds a check
  • Should we allow specifying a different recipient?

Always use safeTransfer | bcf897e

  • Using safeTransfer but not using safeApprove
  • Can quickly change to remove all safe functions as they are unnecessary

Encode and Decode Library with Fuzz Tests

#24

GalloDaSballo and others added 28 commits November 3, 2024 11:56
Fix: Upscale TS to reduce precision loss
wqMerge branch 'fix-temp-invariant-align-bribes' into fix-audit-and-invariants
To avoid circular dependencies when deploying.
Also:
- remove now redundant check for initial initiatives
- add epoch to initial initiatives
Tweaks to incorporate into core deployment
fix: Add Governance owner as constructor param
@danielattilasimon danielattilasimon merged commit 37f414d into main Nov 13, 2024
2 of 3 checks passed
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.

5 participants