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/audit (Implementing fixes for audit issues) #522

Merged
merged 32 commits into from
Nov 16, 2022

Conversation

ctrlc03
Copy link
Collaborator

@ctrlc03 ctrlc03 commented Sep 16, 2022

Implemented fixes for several audit issues:

  1. Implemented fixes for possible reentrancy attacks.
  2. Amended error messages in Poll and
    PollyProcesorAndTallyer.
  3. Introduced local variable in AccQueue to prevent updating state
    variables in a for loop.
  4. Removed redundant boolean comparisons.
  5. Added SafeERC20 to
    Poll to check the return value of transferFrom.
  6. Directly storing the new root rather than calling getStateAqRoot in Poll.

fix #503
fix #504
fix #505
fix #508
fix #510

Upgraded Contracts to pragma 0.8.10 and refactored the code.

chaosma and others added 8 commits August 26, 2022 18:20
…und"

Inconsistent restriction on voice credit upper bound: remove the upper bound. We still have implicit
bound that voice credit<sqrt(field_size), which should be large enough for normal ERC20 balance
… update

fixing issue "Data are not fully verified during state update"
add onlyOwner modifier to airdrop/airdropTo in TopupCredit.sol
add nothing up my sleeve number into the leaf zero in message queue
1. Implemented fixes for possible reentrancy attacks.
 2. Ameneded error messages in `Poll` and
`PollyProcesorAndTallyer`.
 3. Introduced local variable in `AccQueue` to prevent updating state
variables in a for loop.
 4. Removed redundant boolean comparisons.
 5. Added `SafeERC20` to
`Poll` to check the return value of `transferFrom`.

fix privacy-scaling-explorations#503 fix privacy-scaling-explorations#504 fix privacy-scaling-explorations#505 fix privacy-scaling-explorations#508 fix privacy-scaling-explorations#510
…to state

Removed redundant call `getStateAqRoot` and directly stored the new root to `mergedStateRoot`
Reverted the fix related to updating a state variable in a for loop for AccQueue. Also fixed the
tests by adding the `NOTHING_UP_MY_SLEEVE` hash on the local MaciState instance.
Copy link

@Mikerah Mikerah left a comment

Choose a reason for hiding this comment

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

I noticed that within AccQueue.mergeSubRoots, nextSubRootIndex isn't updated to use the new _nextSubRootIndex temporary variable here.

contracts/contracts/MACI.sol Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Show resolved Hide resolved
contracts/contracts/MACI.sol Show resolved Hide resolved
contracts/contracts/Poll.sol Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Show resolved Hide resolved
core/ts/MaciState.ts Outdated Show resolved Hide resolved
…s-interaction pattern

Moved the `_pubKey` check above before increasing the number of signups.

fix privacy-scaling-explorations#503
…hout enqueing new leaves

Added a test that calls the onchain contract to enqueue without adding new data first, to confirm
that the same root is returned.
…hout enqueing new leaves

Added a test that calls the onchain contract to enqueue without adding new data first, to confirm
that the same root is returned.
@ctrlc03 ctrlc03 linked an issue Sep 30, 2022 that may be closed by this pull request
Fixed failing core tests for MaciState.
ctrlc03 and others added 4 commits October 17, 2022 21:06
…y tree

fix circuits folder tests by adding nothing_up_to_my_sleeve for empty tree
test(fix circuits folder tests)
Removed unneeded creation of new message trees, and used the MaciState Poll ones.
@baumstern

This comment was marked as resolved.

@baumstern baumstern linked an issue Nov 7, 2022 that may be closed by this pull request
ctrlc03 and others added 11 commits November 8, 2022 18:16
Refractoring of tests. Still one test erroring out on the circuit side
…te.ts as well as unit tests

(1) fix nothing_up_to_my_sleeve bug in MaciState.ts (2) fix processMessage unit test in circuits
folder
fix(core/ts/macistate.ts): fix nothing_up_to_my_sleeve bug in MaciSta…
Fixed test cases for circuits and other packages.
…leaf

(1) fix issue when insert NOTHING_UP_TO_MY_SLEEVE as first msg leaf (2) refactor part of Poll.sol
(3) catch decryption error during genMaciState if the message is incorrectly encrypted
fix(insert placeholder leaf): insert place holder leaf
Merged Chao fixes for correct enqueuing of the NOTHING_UP_MY_SLEEVE hash and amended Contract tests to fix the final issue.
Upgraded the contracts to solidity 0.8.10, refactored code and added some more tests

fix privacy-scaling-explorations#540
Revert the path to resolve circom binary path from CI side
@baumstern

This comment was marked as resolved.

Revert changes on PollyProcessorAndTallyer
@ctrlc03 ctrlc03 requested a review from daodesigner November 16, 2022 00:19
@daodesigner
Copy link
Contributor

PptE11 is ERROR_INVALID_SUBSIDY_PROOF. Not sure why the proof would be ok offchain but not verify onchain. Make sure vks /inputs line up. @chaosma could you take a look? Really would like to ship with the subsidy feature.

@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Nov 16, 2022

PptE11 is ERROR_INVALID_SUBSIDY_PROOF. Not sure why the proof would be ok offchain but not verify onchain. Make sure vks /inputs line up. @chaosma could you take a look? Really would like to ship with the subsidy feature.

@daodesigner I fixed by reverting some changes I made to the PPT contract, these were mostly gas optimizations which must have broken something.

@ctrlc03 ctrlc03 merged commit f2cef96 into privacy-scaling-explorations:v1 Nov 16, 2022
"eslint": "^6.8.0",
"lerna": "^4.0.0",
"eslint": "^8.27.0",
"lerna": "^6.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment