[M01] Complicated state updates #539
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
SidSethi
commented
Jun 22, 2020
•
SidSethi
commented
- Governance submitProposalVote vote update refactors
- DelegateManager stake balance update refactors
SidSethi
force-pushed
the
ss-feedback-gov
branch
from
977ba5d
to
e7c1d45
Compare
SidSethi
added
audit-feedback
SidSethi
force-pushed
the
ss-feedback-gov
branch
from
e7c1d45
to
9a92cf2
Compare
hareeshnagaraj
approved these changes
Jun 25, 2020
SidSethi
added a commit
that referenced
this pull request
Aug 18, 2020
* [Eth-Contracts] Multiple endpoint support (#273) * First pass of multiple endpoints in SP contracts * Includes unit test validation * [eth-contracts] - Dynamic Staking Bounds + SP Type Whitelist (#287) * Hardcoded list of SP types and associated min/max bounds * Unit tests to confirm min/max threshold behavior * Unit test for multiple endpoints / account / type * Minor fix * [eth-contracts] Update Service Provider endpoint in staking contracts (#311) * [eth-contracts] - Automated claim fund + minted tokens (#300) * [eth-contracts] Stake multiplier model (#330) * Prerequisite for continued delegation work * Switch stake tracking model to use a global multiplier that is incremented as funds are added * All existing tests passing with new code * Merged master * [eth-contracts] Delegation v0 + Governance v1 (#340) * Delegation with rewards split functionality * Stake for SP direct tracked in ServiceProviderFactory, delegate stakes now tracked in DelegateManager * Claims forwarded through DelegateManager, slash handled by delegateManager as well * Revamped reward functionality, with proportional on-demant mint of tokens when claim issued * Undelegate request and lockup added * Governance with vote functionality now working, validated with slash * Convert Staking.sol to new AdminUpgradeabilityProxy + Update all other contracts + migrations + tests (#382) * [eth-contracts] Permission ClaimFactory / DelegateManager / Staking / ServiceProviderFactory (#376) * Enforce callers through registry for all staking functions * Make Governance a RegistryContract (#386) * Replace vendored @openzepplin/upgrades code with package consumption (#388) * [eth-contracts] ServiceProviderFactory ServiceType Operations (#389) * Expose CRUD for known service types to governance or deployer * Convert VersioningFactory to OZ Proxy pattern + rename to VersionTypeManager, Clean up Initializable pattern (#393) * [eth-contracts] Undelegate lockup duration CRUD (#392) * Undelegate lockup duration modification allowed * ClaimFactory to ClaimsManager proxy (#395) * Update claimsManager pattern in governance test (#397) * [eth-contracts] DelegateManager Proxy (#398) * All tests passing, adding isInitialized next * Migration passing again * Test cleanup * [eth-contracts] Extend ServiceType Manager (#401) * Move service type information to ServiceTypeManager * Versioner address renamed to controllerAddress * [eth-contracts] ServiceProviderFactory -> Proxy (#403) * pass 1 of moving stuff out * Tests passing after stg removed everywhere * Move endpoint to own point * SP factory to proxy * Further refactors * Test fix * [eth-contracts] Solidity coverage (#406) * Enable solidity code coverage * [eth-contracts] ServiceProviderFactory Cleanup (#405) * Eliminate duplicate functions, serve details as a single tuple * Update test references * [eth-contracts] Optimizer, dependency updates, upgradeability updates, refactors (#407) * [eth-contracts] Increase test coverage (#412) * Increasing coverage * [eth-contracts] Vendored contract removal + StakingInterface consumption (#414) * Removing TimeHelpers * Removing unnecesary contracts * Use ERCStakingInterface everywhere * Remove import * Adding solcover.js * interface rename, gitignore, governance interface consumption * [eth-contracts] DelegateManager added functionality (#413) * Governance improvements, solc / evmVersion bump, vendored contract removal (#422) * [eth-contracts] ClaimFactory fundingAmount modifier (#420) Expose controllerAddress ability to update fundingAmount Remove references to treasuryAddress where invalid * [eth-contracts] Mainnet update from master (#423) * Pull in missing commits from master * [eth-contracts] Consume Aragon checkpointing (#424) * Use updated Checkpointing from https://www.npmjs.com/package/@aragon/court, code has been audited already * New AudiusAdminUpgradeabilityProxy wrapper + contract upgrade via governance (#425) * [eth-contracts] Finalize move to AudiusAdminUpgradeabilityProxy (#426) * [eth-contracts] AudiusAdminUpgradeabilityProxy in Tests (#427) * Migrations to proxy, Governance guardian veto proposal + execute transaction + test cleanup (#434) * [eth-contracts] Staking - remove unused bytes field, consume openzeppelin test helpers (#428) * [eth-contracts] Update with master (#438) * env var to optionally turn optimizer off (#440) * The 'cse' flag in the optimizer causes code coverage to break. So added an env var which optionally disables the compiler during coverage tests. Interestingly the README for the code coverage says it doesn't use the optimized builds(https://github.com/sc-forks/solidity-coverage/blob/master/README.md#usage-notes), but for some reason it's still fails when we have our optimizer enabled. * [eth-contracts] Update heap size for coverage (#445) * 100% coverage for Registry, RegistryContract, Governance, Initializable, AudiusAdminUpgradeabilityProxy (#451) * 100% coverage for Registry, RegistryContract, Governance, Initializable, AudiusAdminUpgradeabilityProxy * [eth-contracts] Staking, Service Coverage + Minor Contract Changes (#453) * [eth-contracts] Update working branch with master (#460) * Enforce non-empty signature + test cleanup + require statement format (#457) * [eth-contracts] Decrease stake request time delay (#463) * Enforce time difference for decrease stake requests * CRUD for lockup duration * SafeMath for ServiceProviderFactory * [eth-contracts] Remove StakingInterface + Flatten (#467) * [eth-contracts] versionIsValid & updateFundingRoundBlockDiff (#442) * Test validation * Function re-ordering + re-naming + versionIsValid + updateFundingRoundBlockDiff Co-authored-by: Sid Sethi <sidsethi93@gmail.com> * [eth-contracts] Current round logic (#477) * Separate 'current round' state from configuration variables, enabling fundingAmount modification during ongoing round * Enable crytic (Slither only) (#480) * Add safemath everywhere (#473) * Add SafeMath everywhere we use uints * run coverage in circleci * last one didnt work. run coverage in circleci * try again for run coverage in circleci * break lint into a top level task in circle * forgot these files * lint * ClaimsManager.sol using SafeMath * re-enable tests + lint * Governance.sol * Few more contracts * This is causing some problems * this makes the stack too deep error go away * Lets see if coveralls works * comment * wrong place * continue on * specify coverage path * maybe it's the initial period * string? * Stupid period * Add coverage badge to README * Remove this before I forget * This is an actual bug * BIG bug fix * Final conversion for DelegateManager * Remove return declaration * Remove unused Safemath * Round 1 of fixes from Crytic (#481) * Remove shadowed vars * Acknowledge possible re-entrancy bug * Fix re-entrancy possibility here * Move the comment * [WIP] Crytic - round 2 fixes (#482) * Remove explicit false check * Re-entrancy (non-token transfer) in Registry * Cleanup * Limit voting to only Yes/No votes * Stupid * Remove controllerAddress + remove mockGovernance + update all tests to use new governance override pattern" (#485) * [eth-contracts] ServiceProviderFactory ClaimPending Restrictions (#484) * Enforce stake restrictions if a claim is pending for a given service provider * [eth-contracts] Deprecate internal registry consumption (#491) Remove all protocol references to the registry * #494 * #493 * #495 * #490 * Crytic round 3 (#488) * Unused function calls * Revert * Test fix * Reduce unused returns * Try to abstract some stuff without changing order * Remove spFactory double init * Move some stuff around * Remove unused variable (#496) * Remove registry from service type manager, migrations and mock contracts (#497) * ClaimsManager should be 100% now (#499) * [eth-contracts] ServiceProviderFactory init fix + Coverage (#498) * Increased coverage for ServiceTypeManager, ServiceProviderFactory, Staking. DelegateManager * Fix registry governance pattern + change guardianExecTx to revert on tx failure (#500) * Add more coverage for Governance and ServiceTypeManager (#501) * More tests * bug fix * this is why you don't copy paste * [eth-contracts] Imprecise arithmetic operations (#502) * Restructure spDeployerCut calculation to address crytic * [eth-contracts] Ganache test to Istanbul (#503) * Add token & registry governance tests + remove RegistryInterface (#505) * [eth-contracts] Unused return value + constants (#504) * Fix unused return value in validateBalance flow, https://crytic.io/AudiusProject/audius-protocol/117/security_checks/mlt8Dl0gQ5Gxt-oxTB3vDA * Update default values for constants - creator node type min stake - 200,000 AUD creator node type max stake - 3,000,000 AUD discovery provider type min stake - 200,000 AUD discovery provider type max stake - 2,000,000 AUD fundingRoundBlockDiff - 46523 blocks fundingAmount - 1342465.75342 AUD * Add NATSPEC comments (#486) * some comments * More comments * more comments * docs for ContractManager.sol * Two more contracts to go * ServiceTypeManager.sol * Staking.sol should be done * Lint * Update with most recent mainnet changes * lint fixes * AudiusAdminUpgradeability comment fixes * Lints * Claims fix * DelegateManageer comments update * Minor * Migration comment * Commenit Co-authored-by: Sid Sethi <sidsethi93@gmail.com> Co-authored-by: Hareesh Nagaraj <hareesh.nagaraj@gmail.com> * [eth-contracts] Strict equality + local variable shadowing (#506) * Strict equality * Local shadowing * Commenit * Stale TODO removal * [C01] A malicious delegator can permanently lock all stake and rewards for a victim service provider and all of its honest delegators (#561) * Reset locked stake for SP on cancel undelegate stake + test * [C02] Rewards pay out incorrectly when the service provider has a pending “decrease stake” request (#562) * [C03] Updating or removing a service type causes critical accounting errors (#555) * [M01] Complicated state updates (#539) * Governance submitProposalVote vote update refactors * DelegateManager stake balance update refactors * [H01] A malicious delegator can prevent all other delegators from delegating (#552) * Enforce minimum delegation amount for each SP a delegator delegates to instead of across the delegator account * Removal of unused delegatorStakeTotal as it is no longer needed * Additional test case added to verify behavior * [H08] Unresponsive service provider locks delegator stake (#556) * Allow claiming of reward to be callable by anyone, preventing delegators from being stuck when an SP is unresponsive * [H02] Delegators can prevent service providers from deregistering endpoints (#570) * Enabling service providers to forcibly remove a delegator * If maximum bounds are violated due to a delegator stake, SP can forcibly remove their stake and return funds, enabling deregistration * Test case to confirm functionality * [H06] Endpoint registration can be frontrun (#573) * Increase decreaseStakeLockupDuration from 10blocks to 1week * Make decreaseStakeLockupDuration init param so tests can run with lower value than migration * [H07] A service provider can prevent their delegators from undelegating their stake (#577) * Enforce minimum account balance for service provider only, excluding delegator balance (per audit suggestion) * Ensures delegate operations can never force a service provider below the minimum stake value as they are forced to maintain the minimum in direct stake * Corresponding test fixes * Add event emission to Governance functions setRegistryAddress() & transferGuardianship() (#563) * [H05] The quorum requirement can be trivially bypassed with sybil accounts (#574) * Change votingQuorum to votingQuorumPercent + update evaluateProposalOutcome logic + update migration * Add mul overflow comment + fix all governance tests + add test for new quorum check * [H04] No incentive for evaluating proposals with outcome other than Yes (#575) * Add bounded InProgressProposals array and block new proposal submission if evaluatable proposals exist or if array bounds reached * Add test for maxInProgressProposals val + fix previous tests * Make inProgressProposalsAreUpToDate() function publicly accessible + add tests * Make maxInProgressProposals value uint16 instead of uint8, so we can handle more than 255 concurrent proposals * Remove comment * Fix test + comment logs * Restrict function mutability * [M05] Only active stakers can evaluate proposals * Enable any party to evaluate proposal, corresponding test case updated * [M09] Ties in governance proposals are approved * Ties are now reject, corresponding test case added * [M05] Only active stakers can evaluate proposals (follow-up) * Fix compilation error introduced by https://github.com/AudiusProject/audius-protocol/pull/572/files * [M08] Service providers and delegators can mistakenly soft-lock their own stake (#567) * Reject decreaseStake / undelegate requests for amount of zero, eliminating soft-lock conditio * Corresponding test case * [M03] Lack of event emission after sensitive changes (#583) * Events added for key state changes, test verification added for each new event * [M02] Inconsistently checking initialization (#587) * Add requireIsInitialized() check to all functions + update mutability as needed * [M07] Semantic overloading in the No outcome of proposals (#579) * Add new Veto state to eliminate duplicate 'No' outcome * Corresponding test updated * [M06] Voting period and quorum can be set to zero (#568) * Voting period and quorum values checked prior to setting * Corresponding test case * Contracts have artifacts leftover from earlier test versions (#551) * No-op PR, changes addressed in M03 (event emission PR) * [L02] Duplicated newProposalId calculation * [L06] Misleading comments, docstrings, and typographical errors (#546) Co-authored-by: Hareesh Nagaraj <hareesh.nagaraj@gmail.com> * [L03] Duplicated staker check (#580) * Approach detailed here, opted to implement 'isCurrentStaker' only * [M04] Lack of input validation (#569) Audit feedback applied here, note that all setGovernanceAddress/initialize functions within protocol now perform the same isGovernanceAddress check (slight deviation from audit feedback in PR). Coverage drop will be addressed independently. * [N05] Declaring two variables for some addresses #590 * Remove duplicate address tracking * [M10] Some state variables are not set during initialize (#589) * Documenting state variables that are not set during initialize + requiring they are set in all relevant functions * Lint * Removing unnecessary things * Require statement abstraction Co-authored-by: Hareesh Nagaraj <hareesh.nagaraj@gmail.com> * [M02 - pt2 - please see pt1 PR] Inconsistently checking initialization (#594) * [N02] Lack of explicit visibility in public state variables (#592) * [N06] Not declaring uint as uint256 (fix) (#593) * Explicitly declare all instances of uint as uint256 * [L08] Votes can be overwritten by calling the same function for the first vote (#596) * Split submitProposalVote into submit and update functions * Fix require stmt msgs + tests * [N04] Renaming suggestions (#595) * ClaimsManager + ServiceProviderFactory rename * Governance changes * Additional ClaimsManager naming fix * Lint Co-authored-by: Sid Sethi <sidsethi93@gmail.com> * [N01] Inconsistent style for error messages (#597) * Consistent format for error messages with "ContractName: errorMessage" * Any message used >1x within a contract is declared as a private string const to avoid duplication * [N03] Named return variables (#600) * All functions with a single return value no longer use a named return variable * Functions returning a tuple explicitly use named return variables * [L09] README file missing important information (#601) * Eth contracts readme update * Update top-level readme * Add security section regarding responsible disclosure to top-level readme * [L01] Governance proposal description only stored in log (#550) * Governance proposal description only stored in log * Enforce maxDescriptionLength on proposal descriptions + make value governable * Standardize require statements + remove .only in test * WIP * Remove named return variable Co-authored-by: Sid Sethi <sidsethi93@gmail.com> * Update to 0.5.17 (#602) * Update solc version to 0.5.17 * Remove unnused var from and add natspec to InitializableV2 (#604) * Remove unnused var from and add natspec to InitializableV2 * Revert test file changes * [H04 - pt 2 of 2] No incentive for evaluating proposals with outcome other than Yes (#609) * Fix bug where veto proposal halts all proposal actions by failing to clear vetoed proposal from inProgressProposals array * Fixes bug where changing target contract address would cause deadlock since any associated proposals would never evaluate * Fix maxInProgressProposals off-by-one issue * Remove unreachable line (#611) * Vulnerability in proposal execution, which allows circumvention of guardian veto (#607) * Add modifiable execution delay to governance * [L10] Proposed target code can change (#605) * Ensure governance enforces consistent contract state between proposal and evaluation * Test case with selfdestruct functioning * Enforce consistent block times + Default undelegate lockup duration update (#612) * Calculate block differences based on 13s, not 15s * Enforce default undelegateLockupDuration value on deploy to match decreaseStake lockup duration * More test coverage in mainnet-audit-feedback (#591) * Incorrect number of args in initialize * Test getInProgressProposals in Governance * Check setMaxInProgressProposals for invalid value * Fail to add version for invalid service * a non governance address will not be set in ServiceTypeManager * delegate manager setGovernanceAddress contract check test * minimum stake test for claimRewards * Accounts index doesn't work on circle * setGovernanceAddress check in ClaimsManager * Fix test * setGovernanceAddress in Staking contract * Test addresses in between setup * Test fixes * More governance tests * i don't think this does anything * SPFactory init vars * Governance init coverage * test who can remove a delegator * Missing await's * More governance testing * sp factory should be at 100% * Test require fail scenarios in Governance * Partial test working * removeDelegator test without request undelegate stake pending * Remove .only * YATF * fix gov initialize * Simply test case * NIT - logs * new registry test * remove dead comments * Exclude new test contract files (#613) Co-authored-by: Hareesh Nagaraj <hareesh.nagaraj@gmail.com> * [L04] Lack of indexed parameters in events (#614) * Add indexed keyword to event fields in ServiceProviderFactory * ClaimProcessed addressed in prior PR (#583) * Add ability for guardianAddress to submit proposals + add test (#616) * Add ability for guardianAddress to submit proposals + add test * Test fix * [mainnet] Consume UpgradeabilityProxy in AudiusAdminUpgradeabilityProxy (#627) * Eliminate consumption of OZ AdminUpgradeabilityProxy in favor of UpgradeabilityProxy * Reduces unused API surface in AdminUpgradeabilityProxy (changeAdmin, upgradeToAndCall were unsued) in favor of explicit management of lower level contract * Eliminates unnecessary storage of admin + governance address in each proxy contract, and makes governance the administrator * [Audit-Feedback-2] Pull request to aggregate 2nd round of changes (#657) * [N06] Convert rest of uint to uint256 (#649) Original PR https://github.com/AudiusProject/audius-protocol/pull/593/files. * [L05] Contracts have artifacts leftover from earlier test versions (#656) * Original PR https://github.com/AudiusProject/audius-protocol/pull/583/files#diff-97eaf3b56cab95094247b87ff242d988L35-L36 * Remove test token values * [N01] Inconsistent style for error messages (#653) * Standardize any remaining error messages * [N04] Renaming suggestions (#651) * Adding indexed to remaining events (#659) * Original PR https://github.com/AudiusProject/audius-protocol/pull/614/files * Add indexed to the remaining events * [H11] A service provider can prevent their delegators from undelegating their stake (#654) * Remove undelegate bound validation, ensuring a delegator is never stuck * Test case confirmed behavior and fix * [H02] Delegators can prevent service providers from deregistering endpoints (#662) * Old PR - #570 * Adds a time lock mechanism to removeDelegator, ensuring that a service provider cannot maliciously remove delegators to get around slash actions or other protocol enforcement * Enforce relationship between undelegate lockup duration and voting period + execution delay * Enforce relationship between removeDelegator lockup duration and voting period + execution delay * Tests added * Fix typos (#672) * [H10] A service provider can deceive its delegators (#666) * Impose time lock for updating deployer cut * Enforce relationship between update deployer cut time lock and funding round block diff * Enforce relationship between decrease stake lockup and votingPeriod + executionDelay * Additional tests * Comment nits (#677) * Surface level comment changes for additional clarity Co-authored-by: Dheeraj Manjunath <dheerajmanju1@gmail.com> * Preserve audited eth-contracts state Co-authored-by: Hareesh Nagaraj <hareesh.nagaraj@gmail.com> Co-authored-by: Dheeraj Manjunath <dheerajmanju1@gmail.com> Co-authored-by: Roneil Rumburg <roneil@audius.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.