fix: [L-02] Lack of input validation #47

pxrl · 2023-01-02T21:11:34Z

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 input validation to the externally-accessible functions.

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 input validation to the externally-accessible functions.

nicholaspai · 2023-01-02T21:14:08Z

contracts/AcceleratingDistributor.sol

@@ -182,6 +182,7 @@ contract AcceleratingDistributor is ReentrancyGuard, Ownable, Multicall {
     * @param amount The amount of the token to stake.
     */
    function stake(address stakedToken, uint256 amount) external nonReentrant onlyEnabled(stakedToken) {
+        require(amount > 0, "Invalid amount");


Thoughts on moving this check to _stake? to reuse code?

I was anticipating a question about this :)

I thought about applying it to _stake(), but preferred to validate directly on the external function interface, so it's "maximally easy" to for a reviewer check that each of the input arguments are validated, before going any further.

It's of course a fairly simple contract, so it should also be trivially verifiable to require on _stake() instead. That would save a single require() statement. I can see arguments for both, so am definitely open to making adjustments if there is a preference for one or the other.

This isn't a gas savings consideration just a readability and also easier for someone in future to get this require statement "for free" if they use _stake() in the future. I don't think we should be concerned about a reviewer understanding the changes here since the overall diff is so small. So I still think its better to move into _stake() based on your reasoning

nicholaspai

One comment

nicholaspai · 2023-01-02T21:15:25Z

Also small nit: add the "[X tag]" after the conventional commit label fix: <insert here> to work with conventional commit format

Discussed with Nick.

nicholaspai · 2023-01-04T14:51:23Z

test/AcceleratingDistributor.Admin.ts

+        secondsToMaxMultiplier
+      );
+
+      await expect(distributor.connect(owner).stake(lpToken1.address, toWei(0))).to.be.revertedWith(


Cool way to do this test and save code

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.

* 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>

pxrl added the audit label Jan 2, 2023

pxrl requested review from nicholaspai and mrice32 January 2, 2023 21:11

nicholaspai reviewed Jan 2, 2023

View reviewed changes

nicholaspai changed the title ~~[L-02] fix: Lack of input validation~~ fix: [L-02] Lack of input validation Jan 2, 2023

pxrl mentioned this pull request Jan 4, 2023

fix: [L-03] Add missing documentation #53

Merged

improve: Relocate staking amount constraint

53347df

Discussed with Nick.

pxrl requested a review from nicholaspai January 4, 2023 13:53

fix: Un-break test

fc2bcd5

nicholaspai reviewed Jan 4, 2023

View reviewed changes

nicholaspai approved these changes Jan 4, 2023

View reviewed changes

pxrl merged commit d38d4cd into master Jan 4, 2023

pxrl deleted the pxrl/l02-input-validation branch January 4, 2023 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [L-02] Lack of input validation #47

fix: [L-02] Lack of input validation #47

pxrl commented Jan 2, 2023

nicholaspai Jan 2, 2023

pxrl Jan 2, 2023 •

edited

nicholaspai Jan 3, 2023 •

edited

nicholaspai left a comment

nicholaspai commented Jan 2, 2023

nicholaspai Jan 4, 2023

fix: [L-02] Lack of input validation #47

fix: [L-02] Lack of input validation #47

Conversation

pxrl commented Jan 2, 2023

nicholaspai Jan 2, 2023

Choose a reason for hiding this comment

pxrl Jan 2, 2023 • edited

Choose a reason for hiding this comment

nicholaspai Jan 3, 2023 • edited

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

nicholaspai commented Jan 2, 2023

nicholaspai Jan 4, 2023

Choose a reason for hiding this comment

pxrl Jan 2, 2023 •

edited

nicholaspai Jan 3, 2023 •

edited