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

QA Report #269

Open
code423n4 opened this issue May 25, 2022 · 0 comments
Open

QA Report #269

code423n4 opened this issue May 25, 2022 · 0 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

  1. Missing array length match:
    The function below fails to perform input validation on arrays to verify the lengths match.
    A mismatch could lead to an exception or undefined behavior.
    AuraVestedEscrow.fund()  - https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraVestedEscrow.sol#L96
    Booster.voteGaugeWeight() - https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L535

  2. Use of deprecated safeApprove():
    The following functions use the deprecated safeApprove
    AuraBalRewardPool.constructor() - https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraBalRewardPool.sol#L75
    AuraPenaltyForwarder.constructor - https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraPenaltyForwarder.sol#L41
    AuraVestedEscrow._claim()
    CrvDepositorWrapper._setApprovals()

The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code. Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ
As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

  1. Missing approve(0):
    CrvDepositorWrapper.setApproval() - https://github.com/code-423n4/2022-05-aura/blob/main/contracts/CrvDepositorWrapper.sol#L119.

  2. ** UpdatePool should be internal:**
    ConvexMasterChef.updatePool() is currently public instead of internal - https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/ConvexMasterChef.sol#L186

  3. Missing zero address check:
    Aura.constructor()
    AuraVestedEscrow.setAdmin() - missing zero address in constructor and so admin cannot be changed in setAdmin() and setLocker(), cancel() revert
    DepositToken.constructor() - operator parameter , makes contract useless
    PoolManagerProxy.sol - _operator parameter in setOperator(). If owner is malicious, could set operator to zero address and render shutdownPool() and addPool() useless.
    PoolManagerV3sol - _operator parameter in setOperator(). If the current Operator is malicious, operator could set operator to zero address and render shutdownPool() , setProtectPool() and addPool() useless.
    StashFactoryV2.sol - _operator parameter in constructor would require the contract to be redeployed since it would make the contract useless.
    TokenFactory.sol - _operator parameter in constructor would require the contract to be redeployed since it would make the contract useless.
    VirtualBalanceRewardPool.sol - _op param in constructor would make queueNewRewards() revert always.
    cCrv.sol - _operator parameter in setOperator(). If the current Operator is malicious, operator could set operator to zero address and render mint() and burn() useless.

  4. Unbounded loop in function:
    AuraLocker.getReward() includes an unbounded for loop, when the getReward() is called in line 299 and jumps to line 304, may lead to a DoS if the array of rewardsTokens is very large and
    BaseRewardPool.withdraw() - withdrawal may be denied/freeze if the array of extraRewards is too large since the array can keep growing.
    BaseRewardPool.withdrawAndUnwrapTo() - withdrawal may be denied/freeze when the call jumps to _withdrawAndUnwrapTo if the array of extraRewards is too large since the array can keep growing.

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 25, 2022
code423n4 added a commit that referenced this issue May 25, 2022
@0xMaharishi 0xMaharishi added the duplicate This issue or pull request already exists label May 28, 2022
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

3 participants