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

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

QA Report #84

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

Comments

@code423n4
Copy link
Contributor

Parameter validation is lacking zero-address check

Details

Some of the setter function does not check for zero-address
see reference: code-423n4/2021-06-pooltogether-findings#81

Impact

Accidental input of zero-address to _spender would lead to gas wasted.

affected codes:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Velo.sol#L36-L40


Using assert() instead of require()

Details

Some functions used assert instead of require. According to the solidity docs: Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
see reference: code-423n4/2022-01-openleverage-findings#43

Impact

An assert will consume all gas of the transaction whereas a revert/require releases the remaining gas to the transaction sender again.
Usually, one wants to try to keep the gas cost for contract failures low and use assert only for invariants that should always be true.

Mitigation

use require instead of assert()

Affected codes:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L260-L265
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L814-L824
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L828-L840
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L844-L868
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Router.sol#L36
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/RewardsDistributor.sol#L98

@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 29, 2022
code423n4 added a commit that referenced this issue May 29, 2022
@GalloDaSballo
Copy link
Collaborator

Parameter validation is lacking zero-address check

Valid

Using assert() instead of require()

Agree with Non-Critical

1 L, 1 NC

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

2 participants