QA Report #28
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
Missing Zero-address Validation
Severity: Low
Context:
Aura.sol#L45-L53
,Aura.sol#L128-L132
,AuraBalRewardPool.sol#L62-L79
,AuraClaimZap.sol#L68-L83
,AuraLocker.sol#L147-L164
,AuraLocker.sol#L195-L202
,AuraLocker.sol#L205-L212
,AuraLocker.sol#L231-L236
,AuraMerkleDrop.sol#L53-L71
,AuraMerkleDrop.sol#L77-L81
,AuraMerkleDrop.sol#L104-L108
,AuraMinter.sol#L20-L24
,AuraPenaltyForwarder.sol#L30-L42
,AuraStakingProxy.sol#L66-L81
,AuraStakingProxy.sol#L88-L102
,AuraStakingProxy.sol#L137-L140
,AuraStakingProxy.sol#L157-L163
,ArbitratorVault.sol#L31-L35
,BaseRewardPool4626.sol#L31-L41
,Booster.sol#L96-L121
,Booster.sol (For ALL address setters)
,BoosterOwner.sol#L69-L81
,BoosterOwner.sol (For ALL address setters)
,cCrv.sol#L38-L41
,ConvexMasterChef.sol#L78-L88
,CrvDepositor.sol#L47-L60
,CrvDepositor.sol#L62-L65
,DepositToken.sol#L30-L45
,ExtraRewardStashV3.sol#L58-L60
,ExtraRewardStashV3.sol#L69-L76
,PoolManagerProxy.sol#L23-L30
,PoolManagerProxy.sol#L43-L50
,PoolManagerSecondaryProxy.sol#L58-L72
,PoolManagerV3.sol#L29-L38
,RewardFactory.sol#L40-L43
,RewardHook.sol#L32-L35
,StashFactoryV2.sol#L39-L43
,TokenFactory.sol#L31-L39
,VirtualBalanceRewardPool.sol#L110-L117
,VoterProxy.sol#L49-L65
,VoterProxy.sol#L71-L80
Description:
Lack of zero-address validation on address parameters may lead to reverts and force contract redeployments.
Recommendation:
Add explicit zero-address validation on input parameters of address type.
Lack of Event Emission For Critical Functions
Severity: Low
Context:
AuraLocker.sol#L195-L202
,AuraLocker.sol#L205-L212
,AuraStakingProxy.sol#L88-L121
,AuraStakingProxy.sol#L137-L140
,AuraStakingProxy.sol#L157-L163
,ArbitratorVault.sol#L37-L40
,PoolManagerSecondaryProxy.sol#L58-L77
,PoolManagerProxy.sol#L43-L50
,VoterProxy.sol (For ALL setters)
Description:
Several functions update critical parameters that are missing event emission. These should be performed to ensure tracking of changes of such critical parameters.
Recommendation:
Add events to functions that change critical parameters.
Missing Time locks
Severity: Low
Context:
AuraLocker.sol#L195-L202
,AuraLocker.sol#L225-L228
,AuraLocker.sol#L231-L236
,AuraMerkleDrop.sol#L77-L108
,Booster.sol#L351-L386
,PoolManagerProxy.sol#L57-L59
,Description:
None of the onlyOwner functions that change critical protocol addresses/parameters appear to have a time lock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and given them a chance to perform incident response.
Recommendation:
Add a time lock to these functions for a time-delayed change to alert users and protect against possible malicious changes by compromised owners(s).
Missing
onlyOwner
Access ControlSeverity: Low
Context:
AuraLocker.sol#L239-L241
,AuraStakingProxy.sol#L146-L152
Description:
This function is missing an
onlyOwner
Access Control. Since it sets approvals and shouldn't be able to be called by anyone but the owners.Recommendation:
Add an
onlyOwner
Access ControlEvent Spamming
Severity: Low
Context:
Aura.sol#L82-L86
,Description:
The public visibility of this function allows griefing the system via event spamming.
Recommendation:
Add an Access Control to said function to prevent event spamming
Max/Infinite Approvals are Dangerous
Severity: Low
Context:
AuraLocker.sol#L239-L241
,AuraStakingProxy.sol#L146-L152
Description:
Giving max/infinite approvals to contracts is dangerous because if those contracts are exploited then they can remove all the funds from the approving addresses.
Recommendation:
Check allowance and approve as much as required.
Commented Out Code
Severity: Informational
Context:
VirtualBalanceRewardPool.sol#L170
,VirtualBalanceRewardPool.sol#L183
,Description:
Having commented out code is dirty and should be used or removed.
Recommendation:
Delete or use the commented out code.
Missing or Incomplete NatSpec
Severity: Informational
Context:
All Contracts
Description:
Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.
Recommendation:
Add in full NatSpec comments for all functions to have complete code documentation for future use.
Floating Solidity Pragma
Severity: Informational
Context:
./contracts
Description:
Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the pragma (for e.g. by not using
^
in pragma solidity0.5.10
) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.Recommendation:
Remove
^
from in front of your pragma version.ABI Coder v2 Is Activated By Default
>=0.8.0
Severity Informational
Context:
AuraLocker.sol
,Interfaces.sol
Description:
The ABI coder v2 is now activated by default. You can activate the old coder using
pragma abicoder v1
, or explicitly select v2 usingpragma abicoder v2
. ABI coder v2 performs additional checks on the input and supports a larger set of types than v1.Recommendation:
Remove it for clarity.
Too recent of a Pragma
Severity Informational
Context:
./contracts
Description:
Using too recent of a pragma is risky since they are not battle tested. A rise of a bug that wasn't known on release would cause either a hack or a need to secure funds and redeploy.
Recommendation:
Use a Pragma version that has been used for sometime. I would suggest
0.8.4
for the decrease of risk and still has the gas optimizations implemented.Older Version Pragma
Severity: Informational
Context:
./Convex-Platform/contracts
Description:
Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs.
Recommendation:
Consider using a more recent version such as
0.8.4
.Multiple Solidity Pragma
Severity: Informational
Context:
All Contracts
Description:
It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks.
Recommendation:
Ensure all pragma versions are the same one.
The text was updated successfully, but these errors were encountered: