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

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

QA Report #153

code423n4 opened this issue May 21, 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

QA Report

Table of Contents

summary

The main concerns are with the lack of input checks in setters, and the use of deprecated versions of Solidity.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Aura.sol

Aura.sol:89 mint(address _to, uint256 _amount)
Aura.sol:89 minterMint(address _to, uint256 _amount)

AuraClaimZap.sol

AuraClaimZap.sol:85: _checkOption(uint256 _mask, uint256 _flag)

BalLiquidityProvider.sol

BalLiquidityProvider.sol:46: provideLiquidity(bytes32 _poolId, IVault.JoinPoolRequest memory _request) 
BalLiquidityProvider.sol:78: changeMinPairAmount(uint256 _newAmount)
BalLiquidityProvider.sol:88: rescueToken(address _erc20)

ArbitartorVault.sol

ArbitartorVault.sol:46: distribute(address _token, uint256[] calldata _toPids, uint256[] calldata _amounts)

Booster.sol

Booster.sol:547: claimRewards(uint256 _pid, address _gauge)
Booster.sol:558: setGaugeRedirect(uint256 _pid)
Booster.sol:572: _earmarkrewards(uint256 _pid)

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AuraBalRewardPool.sol

AuraBalRewardPool.sol:163: emit Withdrawn(msg.sender, amount); //emitted before getReward(lock) call

AuraLocker.sol

AuraLocker.sol:479: emit DelegateChanged(msg.sender, oldDelegatee, newDelegatee); //emitted before delegate calls for upcoming epoch

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

AuraStakingProxy.sol

AuraStakingProxy.sol:88: setCrvDepositorWrapper()
AuraStakingProxy.sol:99: setKeeper()
AuraStakingProxy.sol:107: setPendingOwner()
AuraStakingProxy.sol:115: applyPendingOwner()
AuraStakingProxy.sol:137: setRewards()

AuraStakingProxy.sol

AuraStakingProxy.sol:77: setAdmin()
AuraStakingProxy.sol:86: setLocker()

ArbitartorVault.sol

ArbitartorVault.sol:37: setOperator()

cCrv.sol

cCrv.sol:38: setOperator()

ConvexMasterChef.sol

ConvexMasterChef.sol:121: set()

CrvDepositor.sol

CrvDepositor.sol:62: setFeeManager()
CrvDepositor.sol:67: setDaoOperator()
CrvDepositor.sol:72: setFees()
CrvDepositor.sol:80: setCoolDown()

ExtraRewardStashV3.sol

ExtraRewardStashV3.sol:145: setRewardHook()
ExtraRewardStashV3.sol:157: setToken()

PoolManagerProxy.sol

PoolManagerProxy.sol:43: setOwner()
PoolManagerProxy.sol:48: setOperator()

PoolManagerSecondaryProxy.sol

PoolManagerSecondaryProxy.sol:58: setOwner()
PoolManagerSecondaryProxy.sol:63: setOperator()
PoolManagerSecondaryProxy.sol:68: setUsedAddress()

PoolManagerV3.sol

PoolManagerV3.sol:40: setOperator()
PoolManagerV3.sol:48: setProtectPool()

StashFactoryV2.sol

StashFactoryV2.sol:45: setImplementation()

VoterProxy.sol

VoterProxy.sol:73: setOwner()
VoterProxy.sol:83: setRewardDeposit()
VoterProxy.sol:73: setSystemConfig()
VoterProxy.sol:105: setOperator()
VoterProxy.sol:116: setDepositor()
VoterProxy.sol:122: setStashAccess()

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AuraBalRewardPool.sol

AuraBalRewardPool.sol:81: totalSupply()
AuraBalRewardPool.sol:85: balanceOf()
AuraBalRewardPool.sol:99: lastTimeRewardApplicable()
AuraBalRewardPool.sol:103: rewardPerToken()
AuraBalRewardPool.sol:113: earned()
AuraBalRewardPool.sol:120: stake()
AuraBalRewardPool.sol:132: stakeAll()
AuraBalRewardPool.sol:138: stakeFor()
AuraBalRewardPool.sol:152: withdraw()

AuraClaimZap.sol

AuraClaimZap.sol:85: getName()

AuraLocker.sol

AuraLocker.sol:781: lastTimeRewardApplicable()
AuraLocker.sol:785: rewardPerToken()
AuraLocker.sol:789: _earned()
AuraLocker.sol:798: _lastTimeRewardApplicable()
AuraLocker.sol:802: _rewardPerToken()
AuraLocker.sol:860: _notifyReward()

AuraMerkleDrop.sol

AuraMerkleDrop.sol:77: setDao()
AuraMerkleDrop.sol:83: setRoot()
AuraMerkleDrop.sol:90: startEarly()
AuraMerkleDrop.sol:96: withdrawExpired()
AuraMerkleDrop.sol:104: setLocker()
AuraMerkleDrop.sol:114: claim()
AuraMerkleDrop.sol:149: forwardPenalty()

CrvDepositorWrapper.sol

CrvDepositorWrapper.sol:51: _setApprovals()
CrvDepositorWrapper.sol:56: _getBptPrice()
CrvDepositorWrapper.sol:67: _getMinOut()
CrvDepositorWrapper.sol:77: _investBalToPool()

ArbitartorVault.sol

ArbitartorVault.sol:37: setOperator()

BaseRewardPool.sol

BaseRewardPool.sol:113: totalSupply()
BaseRewardPool.sol:117: balanceOf()
BaseRewardPool.sol:121: extraRewardsLength()
BaseRewardPool.sol:125: addExtraReward()
BaseRewardPool.sol:132: clearExtraRewards()
BaseRewardPool.sol:147: lastTimeRewardApplicable()
BaseRewardPool.sol:151: rewardPerToken()
BaseRewardPool.sol:165: earned()
BaseRewardPool.sol:173: stake()
BaseRewardPool.sol:185: stakeAll()
BaseRewardPool.sol:191: stakeFor()
BaseRewardPool.sol:222: withdraw()
BaseRewardPool.sol:247: withdrawAll()
BaseRewardPool.sol:251: withdrawAndUnwrap()
BaseRewardPool.sol:260: _withdrawAndUnwrapTo()
BaseRewardPool.sol:276: withdrawAllAndUnwrap()
BaseRewardPool.sol:351: notifyRewardAmount()

BoosterOwner.sol

BoosterOwner.sol:90: transferOwnership()
BoosterOwner.sol:95: acceptOwnership()
BoosterOwner.sol:102: sealOwnership()
BoosterOwner.sol:115: setFactories()
BoosterOwner.sol:119: setArbitrator()
BoosterOwner.sol:123: setFeeInfo()
BoosterOwner.sol:127: updateFeeInfo()
BoosterOwner.sol:131: setFeeManager()
BoosterOwner.sol:135: setVoteDelegate()

ConvexMasterChef.sol

ConvexMasterChef.sol:262: claim()

CrvDepositor.sol

CrvDepositor.sol:62: setFeeManager()
CrvDepositor.sol:67: setDaoOperator()
CrvDepositor.sol:72: setFees()
CrvDepositor.sol:80: setCooldown()
CrvDepositor.sol:205: deposit()
CrvDepositor.sol:209: depositAll()

DepositToken.sol

DepositToken.sol:47: mint()
DepositToken.sol:53: burn()

ProxyFactory.sol

ProxyFactory.sol:11: clone()

StashFactoryV2.sol

StashFactoryV2.sol:11: setImplementation()
StashFactoryV2.sol:87: IsV1()
StashFactoryV2.sol:93: IsV2()
StashFactoryV2.sol:99: IsV3()

TokenFactory.sol

TokenFactory.sol:41: CreateDepositToken()

VirtualBalanceRewardPool.sol

VirtualBalanceRewardPool.sol:133: lastTimeRewardApplicable()
VirtualBalanceRewardPool.sol:137: rewardPerToken()
VirtualBalanceRewardPool.sol:151: earned()
VirtualBalanceRewardPool.sol:207: donate()
VirtualBalanceRewardPool.sol:212: queueNewRewards()
VirtualBalanceRewardPool.sol:236: notifyRewardAmount()

VoterProxy.sol

VoterProxy.sol:122: setStashAccess()
VoterProxy.sol:230: _withdrawSome()
VoterProxy.sol:345: execute()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Tautology

PROBLEM

Statements where a uint256 is checked to be >= 0 should be removed, as a uint256 is always >= 0.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CrvDepositor.sol

CrvDepositor.sol:75: _lockIncentive >= 0

TOOLS USED

Manual Analysis

MITIGATION

Remove the _lockIncentive >= 0 condition

Typos

PROBLEM

There are a few typos in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ArbitartorVault.sol

Arbitartor should be Arbitrator

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BaseRewardPool.sol

BaseRewardPool.sol:214:    uint i
BaseRewardPool.sol:230:    uint i
BaseRewardPool.sol:262:    uint i
BaseRewardPool.sol:296:    uint i

Booster.sol

Booster.sol:379:    uint i

ExtraRewardStashV3.sol

ExtraRewardStashV3.sol:199:    uint i

PoolManagerSecondaryProxy.sol

PoolManagerSecondaryProxy.sol:69:    uint i

TOOLS USED

Manual Analysis

MITIGATION

replace uint with
uint256

Unlocked Solidity compiler pragma

IMPACT

Contracts should be deployed using the same compiler version/flags with which they have been tested.

Using an unlocked pragma of ^0.8.0 which allows different latest versions of Solidity, is risky because it allows contracts to be accidentally deployed using untested newer compiler versions with unfixed/undiscovered bugs

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

All contracts in the contracts folder.

TOOLS USED

Manual Analysis

MITIGATION

Lock the pragmas.

Use an up-to-date version of Solidity

IMPACT

By using a deprecated version of Solidity, you might use some less secure features, and you might get a bigger bytecode out of the compilation process, which will make you pay more gas for deployment.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

All contracts in the convex-platform/contracts folder.

In this case, using a version < 0.8.0 forces you to use the external SafeMath library to check overflow and underflow of additions and subtractions, which could be avoided by upgradi

TOOLS USED

Manual Analysis

MITIGATION

Change the versions from 0.6.12 to any version > 0.8.0

SafeApprove is deprecated

PROBLEM

The safeApprove method from openZeppelin's safeERC20 library is used although it's deprecated.
(see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)

SEVERITY

Non-critical

PROOF OF CONCEPT

It is used in the following contracts:

AuraBalRewardPool.sol

AuraClaimZap.sol

AuraLocker.sol

AuraMerkleDrop.sol

AuraPenaltyForwarder.sol

AuraStakingProxy.sol

AuraVestedEscrow.sol

BalLiquidityProvider.sol

CrvDepositorWrapper.sol

BaseRewardPool4626.sol

Booster.sol

CrvDepositor.sol

VoterProxy.sol

TOOLS USED

Manual Analysis

MITIGATION

As per OZ's suggestions, you should use safeIncreaseAllowance and
safeDecreaseAllowance instead.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

The following setters set an admin but do not check the argument is not the zero address:

AuraMerkleDrop.sol

AuraMerkleDrop.sol:165:setDao

AuraStakingProxy.sol

AuraStakingProxy.sol:88:setCrvDepositorWrapper
AuraStakingProxy.sol:99:setKeeper
AuraStakingProxy.sol:137:setRewards

AuraVestedEscrow.sol

AuraVestedEscrow.sol:165:setAdmin

ArbitartorVault.sol

ArbitartorVault.sol:37:setOperator

Booster.sol

Booster.sol:128:setOwner
Booster.sol:138:setFeeManager
Booster.sol:148:setPoolManager
Booster.sol:181:setArbitrator
Booster.sol:191:setVoteDelegate
Booster.sol:294:setTreasury

cCrv.sol

cCrv.sol:38:setOperator

CrvDepositor.sol

CrvDepositor.sol:62:setFeeManager
CrvDepositor.sol:67:setDaoOperator
CrvDepositor.sol:72:setFees

ExtraRewardStashV3.sol

ExtraRewardStashV3.sol:139:setExtraReward
ExtraRewardStashV3.sol:145:setRewardHook
ExtraRewardStashV3.sol:72:setFees

PoolManagerProxy.sol

PoolManagerProxy.sol:43:setOwner
PoolManagerProxy.sol:48:setOperator

PoolManagerSecondaryProxy.sol

PoolManagerSecondaryProxy.sol:58:setOwner
PoolManagerSecondaryProxy.sol:63:setOperator

PoolManagerV3.sol

PoolManagerV3.sol:40:setOperator

StashFactoryV2.sol

StashFactoryV2.sol:45:setImplementation

VoterProxy.sol

VoterProxy.sol:73:setOwner
VoterProxy.sol:83:setRewardDeposit
VoterProxy.sol:94:setSystemConfig
VoterProxy.sol:105:setOperator
VoterProxy.sol:116:setDepositor

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks
@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 21, 2022
code423n4 added a commit that referenced this issue May 21, 2022
@code423n4
joestakey issue #153
0aa3bc7
@0xMaharishi 0xMaharishi added invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels May 26, 2022
@dmvt dmvt removed the invalid This doesn't seem right label Jul 7, 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Milestone
No milestone
Development

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