fix: [L01] Dangerous maximum values for reward calculations #45

nicholaspai · 2022-12-28T14:46:59Z

L-01 Dangerous maximum values for reward

Description:

The configureStakingToken function in the AcceleratingDistributor contract
contains hard-coded upper limits that ensure maxMultiplier is set below 1e36 and
baseEmissionRate is set below 1e27 . However, these limits are many orders of magnitude
above the intended operating parameters for the contract.
According to the Across documentation for rewards locking, the value of maxMultiplier
should never exceed 3e18 for the initial reward locking program; 1e36 is many orders of
magnitude larger.

The rewards locking documentation also states that the initial token supply for rewards is
75,000,000 ACX tokens. This equates to 7.5e25 wei, but the upper limit of 1e27 on
baseEmissionRate allows a token emission rate per second that is two orders of magnitude
higher than the total supply to be emitted.

Through an accidental or malicious administrative action, maxMultiplier and/or
baseEmissionRate could be set to values that rapidly deplete the rewards pool, allowing
some users to claim excessively large reward payouts until the error is discovered and
corrected.

Consider setting the upper limits for maxMultiplier and baseEmissionRate much
closer to the expected maximum operating values. For example, setting upper limits no more
than 1 order of magnitude above the maximum expected values reduces risk while still
allowing some flexibility to change the terms of the rewards program in the future.

Fix

We set a 1000% upper limit for maxMultiplier (e.g. 10e18) and a 1 million per second upper limit for baseEmissionRate (e.g. 1e6 * 1e18 = 1e24).

Signed-off-by: nicholaspai npai.nyc@gmail.com

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

mrice32

Just a few minor comments!

In general, I think we should avoid making overly strict contract-level restrictions, as this means that contracts have less flexibility once deployed and cause operational issues. On top of that, it means that if we ever wanted to move outside these bounds, we would technically have to modify the source of the contract, which we prefer not to do (even for simple things) without having it re-audited.

I think contract-level restrictions are a double-edged sword that we should use with care.

Thoughts?

mrice32 · 2022-12-29T04:52:33Z

contracts/AcceleratingDistributor.sol

-        require(maxMultiplier < 1e36, "maxMultiplier can not be set too large");
-        require(secondsToMaxMultiplier > 0, "secondsToMaxMultiplier must be greater than 0");
-        require(baseEmissionRate < 1e27, "baseEmissionRate can not be set too large");
+        require(maxMultiplier <= 10e18, "maxMultiplier too large"); // 1000% realistic upper bound.


I think 1e36 is probably a bit ridiculous, but I think 10x multiplier is probably pretty low for a contract-level restriction. I could imagine this contract being used in ways we can't foresee where a larger multiplier over a very long time may be desired.

I think 1,000,000x (i.e. 1e24) might be a decent middle ground. Thoughts?

hmm ok i'm preetty unopinonated and this is many many orders of magnitude more reasonable than 1e36

contracts/AcceleratingDistributor.sol

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

pxrl · 2023-01-04T21:49:00Z

contracts/AcceleratingDistributor.sol

-        require(maxMultiplier < 1e36, "maxMultiplier can not be set too large");
-        require(secondsToMaxMultiplier > 0, "secondsToMaxMultiplier must be greater than 0");
-        require(baseEmissionRate < 1e27, "baseEmissionRate can not be set too large");
+        require(secondsToMaxMultiplier > 0, "secondsToMaxMultiplier <= 0");


Minor nit: secondsToMaxMultiplier is unsigned, so it can only be 0 or positive. Consider "secondsToMaxMultiplier must be non-zero" ?

pxrl

LGTM

* fix: [L01] Dangerous maximum values for reward calculations Signed-off-by: nicholaspai <npai.nyc@gmail.com> * set 1milx max multiplier * Update contracts/AcceleratingDistributor.sol Co-authored-by: Matt Rice <matthewcrice32@gmail.com> * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.Admin.ts Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>

* Certora Acceleration Distributor verification (across-protocol#42) * Initial setup for Acceleration Distributor * Updated main spec * README for Acc Distributor * Acc Dist : Update spec and harness * Update spec. Removed Multicaller interface. * improve: Add distributor contract addresses (across-protocol#40) * Bump decode-uri-component from 0.2.0 to 0.2.2 (across-protocol#41) Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2. - [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases) - [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2) --- updated-dependencies: - dependency-name: decode-uri-component dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * improve: Update MerkleTree import to be consistent with contracts-v2 (across-protocol#43) MerkleTree currently imported in this repo is out of date with latest in @uma/common and @across-protocol/contracts-v2 * improve: Prepare AD Admin test for updates (across-protocol#46) A pending change will revert on amount=0. This breaks a few tests, so prepare updates for those in advance. * fix: [L-02] Lack of input validation (across-protocol#47) The OZ December 2022 audit identified a lack of input validation on the following functions: - stakeFor() (missing validation of beneficiary address). - unstake() (missing validation on the amount input). - _stake() (missing validation on the amount input). This change applies the missing validation in accordance with the recommendations outlined in the finding. * fix: [L-03] Add missing documentation (across-protocol#53) Add documentation for: - UserDeposit and StakingToken structs. - getAverageDepositTimePostDeposit() - getCurrentTime() - _stake() Update documentation for: - Overall contract. - recoverToken() - stake() - stakeFor() - unstake() - withdrawReward() - exit() - _updateReward() * fix: [L-04] Clamp maxMultiplier lower bound to 1e18 (across-protocol#48) Enforce a minimum 1x multiplier to mitigate the potential for underflow in getUserRewardMultiplier(). There's currently no known use case for a maxMultiplier of less than 1x. * improve: [N-01] Remove redundant event parameter (across-protocol#49) The userRewardsOutstanding value is always set to 0 when emitted, so remove it. * fix: [N-02] Misleading documentation (across-protocol#50) Per an OZ finding, the recoverToken() function specifically checks for whether the token has been initialized; not whether it is currently enabled via staking configuration. * improve: [N-03] Improve style guide conformance (across-protocol#52) * fix: [N-04] Minor typographical errors (across-protocol#51) Based on feedback from OZ. * fix: [L01] Dangerous maximum values for reward calculations (across-protocol#45) * fix: [L01] Dangerous maximum values for reward calculations Signed-off-by: nicholaspai <npai.nyc@gmail.com> * set 1milx max multiplier * Update contracts/AcceleratingDistributor.sol Co-authored-by: Matt Rice <matthewcrice32@gmail.com> * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.Admin.ts Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> * improve(AD): [M-01] Clarify staking config change implications (across-protocol#55) Relating to the M-01 finding from OpenZeppelin, the proposal is to document the implications of performing various configuration updates to a pre-existing staking token configuration. * fix: Correct residual typographical errors (across-protocol#57) These should have been handled as part of the following commit, but were not: 5254dec * fix: Reinstate docstrings (across-protocol#56) These were erroneously removed in the following commit: be635c4. * refactor(accelerating-distributor): Remove redundant _updateRewards from exit() (across-protocol#44) * refactor(accelerating-distributor): Remove redundant _updateRewards from exit() Refactors withdrawRewards() and unstake() to call internal methods _withdrawRewards() and _unstake() so that exit() only calls _updateRewards() once * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.sol Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

* improve: Add distributor contract addresses (across-protocol#40) * Bump decode-uri-component from 0.2.0 to 0.2.2 (across-protocol#41) Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2. - [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases) - [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2) --- updated-dependencies: - dependency-name: decode-uri-component dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * improve: Update MerkleTree import to be consistent with contracts-v2 (across-protocol#43) MerkleTree currently imported in this repo is out of date with latest in @uma/common and @across-protocol/contracts-v2 * improve: Prepare AD Admin test for updates (across-protocol#46) A pending change will revert on amount=0. This breaks a few tests, so prepare updates for those in advance. * fix: [L-02] Lack of input validation (across-protocol#47) The OZ December 2022 audit identified a lack of input validation on the following functions: - stakeFor() (missing validation of beneficiary address). - unstake() (missing validation on the amount input). - _stake() (missing validation on the amount input). This change applies the missing validation in accordance with the recommendations outlined in the finding. * fix: [L-03] Add missing documentation (across-protocol#53) Add documentation for: - UserDeposit and StakingToken structs. - getAverageDepositTimePostDeposit() - getCurrentTime() - _stake() Update documentation for: - Overall contract. - recoverToken() - stake() - stakeFor() - unstake() - withdrawReward() - exit() - _updateReward() * fix: [L-04] Clamp maxMultiplier lower bound to 1e18 (across-protocol#48) Enforce a minimum 1x multiplier to mitigate the potential for underflow in getUserRewardMultiplier(). There's currently no known use case for a maxMultiplier of less than 1x. * improve: [N-01] Remove redundant event parameter (across-protocol#49) The userRewardsOutstanding value is always set to 0 when emitted, so remove it. * fix: [N-02] Misleading documentation (across-protocol#50) Per an OZ finding, the recoverToken() function specifically checks for whether the token has been initialized; not whether it is currently enabled via staking configuration. * improve: [N-03] Improve style guide conformance (across-protocol#52) * fix: [N-04] Minor typographical errors (across-protocol#51) Based on feedback from OZ. * fix: [L01] Dangerous maximum values for reward calculations (across-protocol#45) * fix: [L01] Dangerous maximum values for reward calculations Signed-off-by: nicholaspai <npai.nyc@gmail.com> * set 1milx max multiplier * Update contracts/AcceleratingDistributor.sol Co-authored-by: Matt Rice <matthewcrice32@gmail.com> * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.Admin.ts Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> * improve(AD): [M-01] Clarify staking config change implications (across-protocol#55) Relating to the M-01 finding from OpenZeppelin, the proposal is to document the implications of performing various configuration updates to a pre-existing staking token configuration. * fix: Correct residual typographical errors (across-protocol#57) These should have been handled as part of the following commit, but were not: 5254dec * fix: Reinstate docstrings (across-protocol#56) These were erroneously removed in the following commit: be635c4. * refactor(accelerating-distributor): Remove redundant _updateRewards from exit() (across-protocol#44) * refactor(accelerating-distributor): Remove redundant _updateRewards from exit() Refactors withdrawRewards() and unstake() to call internal methods _withdrawRewards() and _unstake() so that exit() only calls _updateRewards() once * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.sol * Update AcceleratingDistributor.sol Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

fix: [L01] Dangerous maximum values for reward calculations

7e39882

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai requested review from mrice32 and pxrl December 28, 2022 14:47

nicholaspai added the audit label Dec 28, 2022

mrice32 reviewed Dec 29, 2022

View reviewed changes

nicholaspai and others added 2 commits January 2, 2023 20:46

set 1milx max multiplier

ceec5ed

Update contracts/AcceleratingDistributor.sol

c0a1a6a

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai requested a review from mrice32 January 3, 2023 01:47

Merge remote-tracking branch 'origin/master' into npai/l01

b6050ea

pxrl reviewed Jan 4, 2023

View reviewed changes

nicholaspai added 2 commits January 4, 2023 16:51

Update AcceleratingDistributor.sol

17d1270

Update AcceleratingDistributor.Admin.ts

c96ce82

pxrl approved these changes Jan 4, 2023

View reviewed changes

nicholaspai merged commit 2e9794a into master Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [L01] Dangerous maximum values for reward calculations #45

fix: [L01] Dangerous maximum values for reward calculations #45

nicholaspai commented Dec 28, 2022 •

edited

mrice32 left a comment

mrice32 Dec 29, 2022

nicholaspai Jan 3, 2023

pxrl Jan 4, 2023

pxrl left a comment

fix: [L01] Dangerous maximum values for reward calculations #45

fix: [L01] Dangerous maximum values for reward calculations #45

Conversation

nicholaspai commented Dec 28, 2022 • edited

L-01 Dangerous maximum values for reward

Description:

Fix

mrice32 left a comment

Choose a reason for hiding this comment

mrice32 Dec 29, 2022

Choose a reason for hiding this comment

nicholaspai Jan 3, 2023

Choose a reason for hiding this comment

pxrl Jan 4, 2023

Choose a reason for hiding this comment

pxrl left a comment

Choose a reason for hiding this comment

nicholaspai commented Dec 28, 2022 •

edited