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 #249

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

QA Report #249

code423n4 opened this issue May 24, 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

Here are QA findings that can be seen throughout the codebase.

[QA-1] Naming inconsistency - arguments at functions or events should have _ at their prefixes

Throughout the codebase, arguments of functions or events have _ at their prefix like function stake(uint256 _amount) or event Recovered(address _token, uint256 _amount).

Following functions or events do not follow this pattern. In addition, some functions or events do not have arguments name defined. It looks like most functions have _ and arguments names defined.

[QA-2] Naming inconsistency - constant variable should be named with capital letters

As solidity document mentions here, it should be named with all capital letters with underscores separating words.

Here are QA findings per file.

Aura.sol

[QA-3] updateOperator function does not properly reflect the comment

updateOperator function has following comment:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/Aura.sol#L80

This can be called if the operator of the voterProxy somehow changes.

The implementation of updateOperator function can be called by anybody and anytime.

If it should reflect the comment if the operator of the voterProxy somehow changes, it should check if newOperator variable is different from the current operator.

function updateOperator() public {
  address newOperator = IStaker(vecrvProxy).operator();
  require(operator != newOperator, "....");

  emit OperatorChanged(operator, newOperator);

[QA-4] Incorrect comment is used at the mint function

The comment inside the mint function says cvs. This seems not correct.

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/Aura.sol#L95-L96

// dont error just return. if a shutdown happens, rewards on old system
// can still be claimed, just wont mint cvx

I assume that it should say AURA instead of cvs.

// dont error just return. if a shutdown happens, rewards on old system
// can still be claimed, just wont mint AURA

[QA-5] AuraMath is not used for some codes

Aura.sol uses AuraMath throughout this file except for the following places.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/Aura.sol#L101

uint256 emissionsMinted = totalSupply() - EMISSIONS_MAX_SUPPLY - minterMinted;

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/Aura.sol#L130

minterMinted += _amount;

For the consistency in Aura.sol, it should use AuraMath as follows.

uint256 emissionsMinted = (totalSupply()).sub(EMISSIONS_MAX_SUPPLY).sub(minterMinted);

minterMinted = minterMinted.add(_amount);

[QA-6] minterMinted private state variable does not have _ at its prefix

Throughout the codebase, private state variables have _ at their prefixes like uint256 private _totalSupply;.

minterMinted private variable does not follow this pattern.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/Aura.sol#L33

uint256 private minterMinted = type(uint256).max;

It should be written like this:

uint256 private _minterMinted = type(uint256).max;

AuraBalRewardPool.sol

[QA-7] _startDelay argument does not have comment

constructor in AuraBalRewardPool contract has comments for arguments, but _startDelay argument does not have the comment starting with @param .

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraBalRewardPool.sol#L54-L69

[QA-8] RewardPaid event does not consider penalty

getReward function gives a staker their reward. However, RewardPaid event is sent with reward.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraBalRewardPool.sol#L187

emit RewardPaid(msg.sender, reward, _lock);

If _lock is false, reward - penalty will be paid to the staker, but RewardPaid event does not reflect this value.

If RewardPaid event should send the proper value which considrs the penalty, it should write like this:

uint256 reward = rewards[msg.sender];
if (reward > 0) {
    rewards[msg.sender] = 0;
    if (_lock) {
        auraLocker.lock(msg.sender, reward);
    } else {
        uint256 penalty = (reward * 2) / 10;
        pendingPenalty += penalty;
        reward = reward - penalty;
        rewardToken.safeTransfer(msg.sender, reward);
    }
    emit RewardPaid(msg.sender, reward, _lock);
}

AuraLocker.sol

[QA-9] Comment at shutdown function seems off

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L224

//shutdown the contract. unstake all tokens. release all locks

It is not clear if calling shutdown function unstakes all tokens and release all locks. I assume that these are behaviors that can happen after shutting the contract.

[QA-10] local variable _rewardsToken has _ at its prefix

Throughout the codebase, local variables do not have _ at its prefix.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L307

address _rewardsToken = rewardTokens[i];

This part should be written like this to be consistent:

address rewardsToken = rewardTokens[i];

AuraVestedEscrow.sol

[QA-11] address(0) check seems to be useful at setAdmin function

If admin becomes address(0), the admin operations can be lost.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L77-L80

    function setAdmin(address _admin) external {
        require(msg.sender == admin, "!auth");
        admin = _admin;
    }

If setting admin = address(0) is not expected, it should add the address(0) check.

    function setAdmin(address _admin) external {
        require(msg.sender == admin, "!auth");
        require(_admin != address(0), "..."); // newly added
        admin = _admin;
    }

BalLiquidityProvider.sol

[QA-12] provider private state variable does not have _ at its prefix

Throughout the codebase, private state variables have _ at their prefixes like uint256 private _totalSupply;.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/BalLiquidityProvider.sol#L19

address private immutable provider;

It should be written like this:

address private immutable _provider;

CrvDepositorWrapper.sol

[QA-13] Naming inconsistency: immutable state variables are named with capital letters

Throughout the codebase, immutable state variables are named in came cases such as address public immutable penaltyForwarder;.

However, following immutable state variables are named with capital letters and not consistent.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/CrvDepositorWrapper.sol#L26-L30

    IVault public immutable BALANCER_VAULT;
    address public immutable BAL;
    address public immutable WETH;
    address public immutable BALANCER_POOL_TOKEN;
    bytes32 public immutable BAL_ETH_POOL_ID;

They should be named as follows:

    IVault public immutable balancerVault;
    address public immutable bal;
    address public immutable weth;
    address public immutable balancerPoolToken;
    bytes32 public immutable balEthPoolId;

[QA-14] Missing comment and empty line at constructor

Following codes have a missing comment and empty line.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/CrvDepositorWrapper.sol#L38-L41

        (
            address poolAddress, /* */


        ) = _balancerVault.getPool(_balETHPoolId);

It can be written like thi:

        (address poolAddress,) = _balancerVault.getPool(_balETHPoolId);

[QA-15] Not setting 0 when calling safeApprove at CrvDepositorWrapper.sol

Throughout the codebase, when safeApprove is being set, 0 is set first and then the actual allowance is set like this:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraClaimZap.sol#L95-L106

    function setApprovals() external {
        require(msg.sender == owner, "!auth");

        IERC20(crv).safeApprove(crvDepositWrapper, 0);
        IERC20(crv).safeApprove(crvDepositWrapper, type(uint256).max);

        IERC20(cvxCrv).safeApprove(cvxCrvRewards, 0);
        IERC20(cvxCrv).safeApprove(cvxCrvRewards, type(uint256).max);

        IERC20(cvx).safeApprove(locker, 0);
        IERC20(cvx).safeApprove(locker, type(uint256).max);
    }

However, _setApprovals function at CrvDepositorWrapper.sol does not follow this pattern.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/CrvDepositorWrapper.sol#L51-L54

    function _setApprovals() internal {
        IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);
        IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);
    }

To be consistent, it is worth following the same pattern like this:

    function _setApprovals() internal {
        IERC20(WETH).safeApprove(address(BALANCER_VAULT), 0);
        IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);

        IERC20(BAL).safeApprove(address(BALANCER_VAULT), 0);
        IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);
    }

Interfaces.sol

[QA-16] Setting pragma abicoder v2 seems not necessary

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/Interfaces.sol#L3

pragma abicoder v2;

If removing the definition of abicoder is allowed, it should remove it. Confirmed that the test passed without pragma abicoder v2.
@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 24, 2022
code423n4 added a commit that referenced this issue May 24, 2022
@code423n4
TerrierLover issue #249
1dac097
@0xMaharishi 0xMaharishi added the duplicate This issue or pull request already exists label May 28, 2022
This was referenced May 29, 2022
Merged
Open
@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
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
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dmvt @code423n4 @0xMaharishi