GalloDaSballo
commented
2 years ago Grammatical Errors
NC
Variable mutability and visibility unspecified
NC
No return type mentioned, yet functions are utilising the return keyword
NC
Variables are implicitly their default types
NC / gas
NatSpec tag missing
NC
Ensure uniformity in how constant variables are written
NC
SetGauge() can be modified to any address by a malicious user thereby affecting key functions
Valid Low
Remove unnecessary copied comments
NC
Short and sweet, 1 L, 7NC
QA
Grammatical Errors
“observations”
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L37
“it's”
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/factories/PairFactory.sol#L20
“be”
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L602
remove “be”
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L330
“written in the”
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L256
Variable mutability and visibility unspecified
One can infer from the uppercase words with an underscore, that this variable should be a constant:
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L25
Since there's no visibility specified, this is implicitly visible but devs need to be explicit in which they want to employ :
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L35
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L45
No return type mentioned, yet functions are utilising the return keyword
No return value mentioned :
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L1335
but delegate() and delegateBySig() dictates the return of some type.
Consider removing the return keyword since it's merely an internal call.
Variables are implicitly their default types
Unnecessary initialisation of a state variable since it's already false and you have a function setPause() that sets the flag:
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/factories/PairFactory.sol#L30
[https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L139-L141
NatSpec tag missing
missing @return
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L782
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L218
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L200
[https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L193
[https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L186
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L162
Ensure uniformity in how constant variables are written
Use uppercase(and or with underscore for more than one word):
https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L108-L109
https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L122-L125
SetGauge() can be modified to any address by a malicious user thereby affecting key functions
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L30
The aforementioned function is callable by anyone and if the gauge is not already set, a malicious individual can set an address controlled by themself.
The gauge is initially set in the constructor of the gauge contract:
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L104
Therefore, it is best to resolve the issue by setting msg.sender to the gauge address.
If a malicious user sets the gauge, they can call addRewardToken(), swapOutReward() and deliverReward(). This last function will enable the malicious party, to send rewards to themself thereby stealing the rewards that would need to be distributed to other users of the protocol.
Remove unnecessary copied comments
After speaking to the dev, there are certain comments that should be removed as this is not the desired behaviour.
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L206