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

fixes all vulnerabilities in the DAO and add tests with 100 percent coverage #36

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

jatZama
Copy link
Member

@jatZama jatZama commented Apr 4, 2024

I fixed all serious vulnerabilities found inside the DAO and made all modifications we discussed on slack. Compare coverage before vs after. All tests are passing in both mocked mode and fhevm mode, however few tests are only run in mocked mode: those were we needed to fast forward time by several days to test the timelock mostly, because we had to use the hardhat cheat codes in this case.
Screenshot 2024-04-05 at 01 26 22

Screenshot 2024-04-05 at 00 19 10
The only contract not 100% covered is Comp because there is a branch that could never be reached, and two functions are almost duplicated: getPriorVotes and getMyPriorVotes while we already have 100% branch coverage for getMyPriorVotes.

There is still the open question of wether I should re-introduce the castVoteBySig function, but for this we would need to add the ability in fhevmjs to interface correctly with the ZKPOK Rust library in order to be able to delegate the ciphertext to a custom tx.origin address. What's your opinion on this @immortal-tofu ?
Another open question is wether I should add virtual modifiers to the functions inside the different contracts, to make it easily extendable and customizable like I did for the EncryptedERC20, wdyt ?

@jatZama jatZama requested a review from immortal-tofu April 4, 2024 23:24
Copy link
Collaborator

@immortal-tofu immortal-tofu left a comment

Choose a reason for hiding this comment

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

LGTM!

@jatZama jatZama merged commit c3ce9ab into main Apr 5, 2024
2 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.

2 participants