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.

Sign up for GitHub

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

Jump to bottom

QA Report #295

Open
code423n4 opened this issue May 25, 2022 · 1 comment
Open

QA Report #295

code423n4 opened this issue May 25, 2022 · 1 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 sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

1. Aura - can mint before initialize

Risk

Low

Impact

Contract Aura.sol is vulnerable to transaction dependence. Minter is able to mint initial tokens via minterMint function before init function is invoked. This makes it impossible to run init function later since totalSupply() will be bigger than 0.

Proof of Concept

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to check if init function has been run before allowing minterMint to be executed. This can be achieved in a similar way as for mint function - require(totalSupply() != 0, "Not initialised");.

2. Missing validation value checks

Risk

Low

Impact

Multiple contracts are missing basic validation checks for function arguments which might lead to undefined behavior.

Proof of Concept

Aura.sol:

AuraMinter.sol:

AuraVestedEscrow.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to check if the passed values are in expected ranges.

3. Not following checks-effects-interactions pattern

Risk

Low

Impact

Multiple contracts do not follow checks-effects-interactions pattern which might lead to reentrancy attacks.

AuraVestedEscrow.cancel:

rewardToken.safeTransfer(admin, delta);
totalLocked[_recipient] = 0;

ClaimFeesHelper.claimFees:

(..)
feeDistro.claimToken(voterProxy, _token);

// Loop through until something is transferred
while (IERC20(_token).balanceOf(voterProxy) <= bal) {
    feeDistro.claimToken(voterProxy, _token);
}

booster.earmarkFees(address(_token));
lastTokenTimes[address(_token)] = tokenTime;

Proof of Concept

AuraVestedEscrow.sol:

ClaimFeesHelper.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to first set the effects and then perform interactions such as external calls.

4. Hardcoded decimals

Risk

Low

Impact

Contracts AuraBalRewardPool.sol, AuraLocker.sol and CrvDepositorWrapper.sol are using hardcoded number of decimals - 1e18. It is better to dynamically read the value of decimals via _decimals or decimals().

Proof of Concept

AuraBalRewardPool.sol:

AuraLocker.sol:

CrvDepositorWrapper.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to dynamically read the value of decimals _decimals or decimals().

5. Missing zero address checks

Risk

Low

Impact

Multiple contracts of Aura do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

Aura.sol:

AuraBalRewardPool.sol:

AuraClaimZap.sol:

AuraLocker.sol:

AuraMerkleDrop.sol:

AuraMinter.sol:

AuraPenaltyForwarder.sol:

AuraStakingProxy.sol:

AuraVestedEscrow.sol:

BalLiquiudityProvider.sol:

ClaimFeesHelper.sol:

CrvDepositorWrapper.sol:

ExtraRewardsDistributor.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

6. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

AuraClaimZap.sol:

AuraStakingProxy.sol:

AuraVestedEscrow.sol:

ClaimFeesHelper.sol:

CrvDepositorWrapper.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

7. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

AuraLocker.sol:

AuraMerkleDrop.sol:

AuraMinter.sol:

AuraVestedEscrow.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing critical addresses.

8. The contracts use unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

Contracts in scope use unlocked pragma:

^0.8.11 - Aura.sol
^0.8.11 - AuraBalRewardPool.sol
^0.8.11 - AuraClaimZap.sol
^0.8.11 - AuraLocker.sol
^0.8.11 - AuraMath.sol
^0.8.11 - AuraMerkleDrop.sol
^0.8.11 - AuraMerkleDrop.sol
^0.8.11 - AuraMinter.sol
^0.8.11 - AuraPenaltyForwarder.sol
^0.8.11 - AuraStakingProxy.sol
^0.8.11 - AuraVestedEscrow.sol
^0.8.11 - BalLiquidityProvider.sol
^0.8.11 - ClaimFeesHelper.sol
^0.8.11 - CrvDepositorWrapper.sol
^0.8.11 - ExtraRewardsDistributor.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Consider using a single compiler version for compiling both contracts, for example 0.8.11.

9. Contracts use different compiler versions

Risk

Non-Critical

Impact

Using different compiler versions across contracts of the same project might lead to confusion and accidental errors.

Proof of Concept

Examples:

0.8.11 - contracts/Interfaces.sol
^0.8.11 - contracts/*
0.6.12 - convex-platform/contracts/contracts/*

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps
Consider using a single compiler version for compiling both contracts, for example 0.8.10

10. Deprecated safeApprove

Risk

Non-Critical

Impact

Functioin safeApprove has been deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().

Proof of Concept

AuraBalRewardPool.sol:75:        rewardToken.safeApprove(_auraLocker, type(uint256).max);
AuraClaimZap.sol:98:        IERC20(crv).safeApprove(crvDepositWrapper, 0);
AuraClaimZap.sol:99:        IERC20(crv).safeApprove(crvDepositWrapper, type(uint256).max);
AuraClaimZap.sol:101:        IERC20(cvxCrv).safeApprove(cvxCrvRewards, 0);
AuraClaimZap.sol:102:        IERC20(cvxCrv).safeApprove(cvxCrvRewards, type(uint256).max);
AuraClaimZap.sol:104:        IERC20(cvx).safeApprove(locker, 0);
AuraClaimZap.sol:105:        IERC20(cvx).safeApprove(locker, type(uint256).max);
AuraLocker.sol:240:        IERC20(cvxCrv).safeApprove(cvxcrvStaking, 0);
AuraLocker.sol:241:        IERC20(cvxCrv).safeApprove(cvxcrvStaking, type(uint256).max);
AuraMerkleDrop.sol:131:            aura.safeApprove(address(auraLocker), 0);
AuraMerkleDrop.sol:132:            aura.safeApprove(address(auraLocker), _amount);
AuraPenaltyForwarder.sol:41:        token.safeApprove(address(distributor), type(uint256).max);
AuraStakingProxy.sol:147:        IERC20(crv).safeApprove(crvDepositorWrapper, 0);
AuraStakingProxy.sol:148:        IERC20(crv).safeApprove(crvDepositorWrapper, type(uint256).max);
AuraStakingProxy.sol:150:        IERC20(cvxCrv).safeApprove(rewards, 0);
AuraStakingProxy.sol:151:        IERC20(cvxCrv).safeApprove(rewards, type(uint256).max);
AuraStakingProxy.sol:215:            _token.safeApprove(rewards, 0);
AuraStakingProxy.sol:216:            _token.safeApprove(rewards, type(uint256).max);
AuraVestedEscrow.sol:186:            rewardToken.safeApprove(address(auraLocker), claimable);
BalLiquidityProvider.sol:59:            tkn.safeApprove(address(bVault), 0);
BalLiquidityProvider.sol:60:            tkn.safeApprove(address(bVault), bal);
CrvDepositorWrapper.sol:52:        IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);
CrvDepositorWrapper.sol:53:        IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to consider using safeIncreaseAllowance() and safeDecreaseAllowance() functions instead of safeApprove.

11. Usage of boolean values in expressions

Risk

Non-Critical

Impact

Protocol uses false boolean expression for require statement in functions.

Proof of Concept

AuraMerkleDrop.sol:123     require(hasClaimed[msg.sender] == false, "already claimed");

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove false expression:
require(!hasClaimed[msg.sender], "already claimed");

12. Each event should use three indexed fields if there are three or more fields

Risk

Non-Critical

Impact

Each event should use three indexed fields if there are three or more fields.

Proof of Concept

AuraBalRewardPool.sol:48:    event RewardAdded(uint256 reward);
AuraBalRewardPool.sol:49:    event Staked(address indexed user, uint256 amount);
AuraBalRewardPool.sol:50:    event Withdrawn(address indexed user, uint256 amount);
AuraBalRewardPool.sol:51:    event RewardPaid(address indexed user, uint256 reward, bool locked);
AuraBalRewardPool.sol:52:    event PenaltyForwarded(uint256 amount);
AuraLocker.sol:126:    event Recovered(address _token, uint256 _amount);
AuraLocker.sol:127:    event RewardPaid(address indexed _user, address indexed _rewardsToken, uint256 _reward);
AuraLocker.sol:128:    event Staked(address indexed _user, uint256 _paidAmount, uint256 _lockedAmount);
AuraLocker.sol:129:    event Withdrawn(address indexed _user, uint256 _amount, bool _relocked);
AuraLocker.sol:130:    event KickReward(address indexed _user, address indexed _kicked, uint256 _reward);
AuraLocker.sol:131:    event RewardAdded(address indexed _token, uint256 _reward);
AuraLocker.sol:133:    event KickIncentiveSet(uint256 rate, uint256 delay);
AuraMerkleDrop.sol:36:    event DaoSet(address newDao);
AuraMerkleDrop.sol:37:    event RootSet(bytes32 newRoot);
AuraMerkleDrop.sol:39:    event ExpiredWithdrawn(uint256 amount);
AuraMerkleDrop.sol:40:    event LockerSet(address newLocker);
AuraMerkleDrop.sol:41:    event Claimed(address addr, uint256 amt, bool locked);
AuraMerkleDrop.sol:42:    event PenaltyForwarded(uint256 amount);
AuraPenaltyForwarder.sol:22:    event Forwarded(uint256 amount);
AuraStakingProxy.sol:53:    event RewardsDistributed(address indexed token, uint256 amount);
AuraStakingProxy.sol:54:    event CallIncentiveChanged(uint256 incentive);
AuraVestedEscrow.sol:38:    event Funded(address indexed recipient, uint256 reward);
AuraVestedEscrow.sol:40:    event Claim(address indexed user, uint256 amount, bool locked);
BalLiquidityProvider.sol:24:    event LiquidityProvided(uint256[] input, uint256 output);
BalLiquidityProvider.sol:25:    event MinPairAmountChanged(uint256 oldMinPairAmount, uint256 newMinPairAmount);
ExtraRewardsDistributor.sol:28:    event RewardAdded(address indexed token, uint256 indexed epoch, uint256 reward);
ExtraRewardsDistributor.sol:29:    event RewardPaid(address indexed user, address indexed token, uint256 reward, uint256 index);
ExtraRewardsDistributor.sol:30:    event RewardForfeited(address indexed user, address indexed token, uint256 index);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add three indexed fields for events if there are three or more fields.

13. Natspec is incomplete

Risk

Non-Critical

Impact

Multiple contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

Aura.sol:

AuraBalRewardPool.sol:

AuraLocker.sol:

AuraMath.sol:

AuraMerkleDrop.sol:

AuraMinter.sol:

AuraStakingProxy.sol:

AuraVestedEscrow.sol:

BalLiquidityProvider.sol:

ClaimFeesHelper.sol:

CrvDepositorWrapper.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

14. Lack of using native time units

Risk

Non-Critical

Impact

Contract AuraLocker.sol is using number of seconds and multiplying to define time units such as day and week. This might lead to confusion and accidental mistakes.

Proof of Concept

AuraLocker.sol:81:    uint256 public constant rewardsDuration = 86400 * 7;

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use native time units such as seconds, minutes, hours, days, weeks and years.
@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
@code423n4
reassor issue #295
147aa8a
@0xMaharishi 0xMaharishi added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label May 28, 2022
@0xMaharishi
Copy link

Comprehensive QA report

@dmvt dmvt mentioned this issue Jul 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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 sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@code423n4 @0xMaharishi