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

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

QA Report #203

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

array length of the input parameters should be checked to be equal in fund() function.

fund() function takes 2 array arguments; address[] _recipient and uint256[] _amount. If the length of the _amount array is smaller than _recipient, below code will revert with array out of boundary error.
for (uint256 i = 0; i < _recipient.length; i++) {
uint256 amount = _amount[i];

Therefore, length of the arrays should be checked initially to be equal.

Lines of code

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

execute() don't revert on failed external call

execute() function within BoosterOwner.sol makes low level call to other contracts. The success return value is received and returned back to the caller. However, the function does not revert if success is false.
Another similar execute() function within VoterProxy.sol reverts if succes is false.

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/BoosterOwner.sol#L187

Use modifiers only for checks

The code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern. Moreover, these statements may also remain unnoticed by the developer, as the code for modifier may be far from the function declaration.
https://consensys.net/blog/developers/solidity-best-practices-for-smart-contract-security/#:~:text=Use%20modifiers%20only%20for%20checks

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraLocker.sol#L170
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraBalRewardPool.sol#L89
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/BaseRewardPool.sol#L137
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol#L123

Inflation protection time is 3 years, but comment states it as 4 years

It is commented within AuraMinter.sol that the AuroToken is protected from inflation for 4 years. However, inflationProtectionTime is set for 3 years (156 weeks). Having communicated with the developer team, I was informed that the comment was wrong.

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraMinter.sol#L9-L10
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraMinter.sol#L23

Floating pragma

Contracts under https://github.com/code-423n4/2022-05-aura/tree/main/contracts folder are using floating pragma (^0.8.11).
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
https://swcregistry.io/docs/SWC-103

Lines of code

All the contracts under https://github.com/code-423n4/2022-05-aura/tree/main/contracts folder, except CrvDepositorWrapper.sol and Interfaces.sol

Different compiler versions used

Contracts under https://github.com/code-423n4/2022-05-aura/tree/main/contracts folder are using ^0.8.11, whereas contracts under https://github.com/code-423n4/2022-05-aura/tree/main/convex-platform/contracts/contracts use 0.6.12.
It is recommended to use the same solidity version for all the contracts.
https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used

address _token can be indexed within Recovered() event.

Other events address parameters through the contracts are indexed, so it might be better to have address _token to be indexed as well.

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraLocker.sol#L126

Typo in comment

Typo at "distributed"

Lines of code

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

Missing check to prevent adding an LP token which already exists in the pool

It is commented before add() function of ConvexMasterChef.sol that the rewards will be messed up if the same LP token is added twice. If there is such a risk, adding an LP token which already exists must be prevented by a control within the function, rather than a warning comment.

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/ConvexMasterChef.sol#L94-L118

Missing non-zero address check

It is a good practice to include non-zero address check especially when updating important addresses. I suggest to include a non-zero address check for below critical address updates.

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L128
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L138
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L148
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L181
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/Booster.sol#L191
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/Aura.sol#L82
https://github.com/code-423n4/2022-05-aura/blob/main/convex-platform/contracts/contracts/cCrv.sol#L38

@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
@0xMaharishi 0xMaharishi added the duplicate This issue or pull request already exists label May 27, 2022
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jul 7, 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