Velodrome Finance contest - TerrierLover's results

A base layer AMM on Optimism, inspired by Solidly.

General Information

Platform: Code4rena

Start Date: 23/05/2022

Pot Size: $75,000 USDC

Total HM: 23

Participants: 75

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 13

Id: 130

League: ETH

Velodrome Finance

Findings Distribution

Researcher Performance

Rank: 37/75

Findings: 2

Award: $153.08

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Velodrome Finance

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x52, 0xNazgul, 0xNineDec, AlleyCat, BouSalman, CertoraInc, Chom, Dravee, Funen, GimelSec, Hawkeye, MaratCerby, Nethermind, Picodes, RoiEvenHaim, SooYa, TerrierLover, WatchPug, _Adam, asutorufos, berndartmueller, c3phas, catchup, cccz, cryptphi, csanuragjain, delfin454000, djxploit, fatherOfBlocks, gzeon, hake, hansfriese, horsefacts, hyh, jayjonah8, minhquanym, oyc_109, p_crypt0, pauliax, robee, rotcivegaf, sach1r0, sashik_eth, simon135, sorrynotsorry, teddav, unforgiven, xiaoming90

Awards

102.2071 USDC - $102.21

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

Here are 8 QA reports

[QA-1] Inconsistent naming - use camel case for state variables and functions

Throughout the codebase, state variables (which are not constants) and functions are named with camel case such as mapping(address => uint) public rewardRate and function claimFees(). However, following state variables and functions do not follow this pattern.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L81

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L86

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L91

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L96

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L101

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L32

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L33

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L34

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L35

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L37

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L38

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L40

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L41

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L42

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L44

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L64

[QA-2] Inconsistent naming - use capital letters with underscores for constant

As solidity document mentions here, it should be named with all capital letters with underscores separating words. However, following state variables and functions do not follow this pattern.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L15

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L45

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L6

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L7

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L8

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L108

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L109

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L122

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L123

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L124

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L125

[QA-3] No return variables need to be defined

Return variables uint amount, bool stable are defined at getAmountOut function, but they are not necessary.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L71

[QA-4] Inappropriate comments exist at Router.sol

Following comment exists at quoteAddLiquidity and quoteRemoveLiquidity and in Router.sol but these functions do not seem to create the pair.

// create the pair if it doesn't exist yet

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L109

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L140

If it does not create the pair, these comments should be removed.

[QA-5] Arguments do not have new lines

Throughout the codebase, arguments are set in new lines if it does not fit within 1 line. However, following codes do not follow this pattern like bool approveMax, uint8 v, bytes32 r, bytes32 s at removeLiquidityWithPermit function.

    function removeLiquidityWithPermit(
        address tokenA,
        address tokenB,
        bool stable,
        uint liquidity,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline,
        bool approveMax, uint8 v, bytes32 r, bytes32 s
    ) external returns (uint amountA, uint amountB) {

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L286

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L305

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L343

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L358

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L386

For the visibility, it is better to place arguments in a new line like this.

    function removeLiquidityWithPermit(
        address tokenA,
        address tokenB,
        bool stable,
        uint liquidity,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline,
        bool approveMax, 
        uint8 v, 
        bytes32 r, 
        bytes32 s
    ) external returns (uint amountA, uint amountB) {

[QA-6] Inconsistent function coding style

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L364-L367

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L378-L380

[QA-7] Unnecessary casting

Casting (uint256 -> uint or uint -> uint256) happens here.

totalWeight -= uint256(_totalWeight);

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L118

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L168-L169

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L295

This casting seems not necessary and can be written like this.

totalWeight -= _totalWeight;

[QA-8] Avoid using assert

According to the solidity doc, using assert should be avoided.

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.

Following codes use assert, but it should be changed into require or others.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L181

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L272

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L508

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L748

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L845

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L861

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L937

#0 - GalloDaSballo
2022-07-04T22:26:48Z

[QA-1] Inconsistent naming - use camel case for state variables and functions

Disagree, this is the pythonista way, which is used consistently for functions ported over from the original vyper code

[QA-3] No return variables need to be defined

Valid NC

[QA-4] Inappropriate comments exist at Router.sol

Valid NC

[QA-5] Arguments do not have new lines

Valid NC

[QA-7] Unnecessary casting

Valid NC

[QA-8] Avoid using assert

Valid Low

Pretty decent report 1 L, 4 NC

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x4non, 0xNazgul, 0xf15ers, 0xkatana, Chom, ControlCplusControlV, DavidGialdi, Deivitto, Dravee, ElKu, Fitraldys, Funen, GimelSec, MadWookie, MaratCerby, MiloTruck, Picodes, Randyyy, TerrierLover, TomJ, Tomio, UnusualTurtle, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, csanuragjain, delfin454000, djxploit, fatherOfBlocks, gzeon, hake, hansfriese, horsefacts, orion, oyc_109, pauliax, reassor, rfa, rotcivegaf, sach1r0, saian, sashik_eth, simon135, supernova, teddav, z3s

Awards

50.8715 USDC - $50.87

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Here are Gas reports

[GAS-1] Use custom errors

Using custom errors can reduce the gas cost.

There are more than 100 callsites of require check with inline error message. Using the custom errors instead of the inline error messages can reduce the gas cost.

[GAS-2] Use `!= 0` instead of `> 0` on uint variables

uint variables will never be lower than 0. Therefore, > 0 and != 0 have same meanings. Using != 0 can reduce the gas deployment cost, so it is worth using != 0 wherever possible.

If the codebase uses > 0 on uint variables like this:

require(_amount > 0, "...");

it can rewrite like this:

require(_amount != 0, "...");

The above change can be applied to the following codes.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L42 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L86 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L93 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L100

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/PairFees.sol#L20 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/PairFees.sol#L29-L30

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L58-L59

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L111 (This if statement seems not necessary since it checks _votes != 0)

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L167 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L227 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L239 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L259 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L289 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L294 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L322 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L352

[Gas-3] No need to set 0 on uint variables

The default value of uint varibles are 0. Therefore, there is no need to set 0 on uint variables. Not setting 0 on uint variables can reduce the gas cost.

If 0 is set on uint variable like this uint256 amount = 0;, it can rewrite to uint256 amount;. In for loop, if uint is used like this for (uint256 i = 0; i < length; i++), it can omit setting = 0 and will be for (uint256 i; i < length; i++).

The above change can be applied to the following codes.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L179 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L353 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L481 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L551

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L57

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L20 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L257 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L273-L274 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L389

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L90 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Router.sol#L316

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L9

[Gas-4] Potential usage of unchecked

Following variables or operations can be wrapped by unchecked.

i++ or x++ https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L405

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L426

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L389

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L75

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L105

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L121

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L148

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L195

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L272

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L340

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L632

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L886 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L942 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L1017

balance / DURATION

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L615

amount / DURATION https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Gauge.sol#L650
priceAverageCumulative / granularity https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L260
y - y_prev and y_prev - y https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L399-L407
temp /= 10, value /= 10 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L138

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L144

[Gas-5] Avoid defining DOMAIN_SEPARATOR every time the functions are called

At folowing functions, they define DOMAIN_SEPARATOR every time the functions are called. DOMAIN_SEPARATOR can be set beforehand, and it can avoid defining DOMAIN_SEPARATOR every time.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Pair.sol#L468-L476

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L1362-L1370

#0 - GalloDaSballo
2022-06-30T01:23:22Z

[Gas-5] Avoid defining DOMAIN_SEPARATOR every time the functions are called

Would save 120 gas and 90 gas per function call respectively

#1 - GalloDaSballo
2022-06-30T01:24:37Z

[Gas-3] No need to set 0 on uint variables

51 entries per instance

[Gas-4] Potential usage of unchecked

[GAS-2] Use != 0 instead of > 0 on uint variables

Is no longer valid

Overall a neat report with an interesting find (5) Would save 20 gas (at least) per instance 20 * 20 = 400

#2 - GalloDaSballo
2022-06-30T01:24:53Z

Total Gas Saved 661

Velodrome Finance contest - TerrierLover's results

A base layer AMM on Optimism, inspired by Solidly.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-48] QA Report - $102.21

Findings Information

Awards

Labels

External Links

[QA-1] Inconsistent naming - use camel case for state variables and functions

[QA-2] Inconsistent naming - use capital letters with underscores for constant

[QA-3] No return variables need to be defined

[QA-4] Inappropriate comments exist at Router.sol

[QA-5] Arguments do not have new lines

[QA-6] Inconsistent function coding style

[QA-7] Unnecessary casting

[QA-8] Avoid using assert

#0 - GalloDaSballo2022-07-04T22:26:48Z

[QA-1] Inconsistent naming - use camel case for state variables and functions

[QA-3] No return variables need to be defined

[QA-4] Inappropriate comments exist at Router.sol

[QA-5] Arguments do not have new lines

[QA-7] Unnecessary casting

[QA-8] Avoid using assert

[G-16] Gas Report - $50.87

Findings Information

Awards

Labels

External Links

[GAS-1] Use custom errors

[GAS-2] Use != 0 instead of > 0 on uint variables

[Gas-3] No need to set 0 on uint variables

[Gas-4] Potential usage of unchecked

[Gas-5] Avoid defining DOMAIN_SEPARATOR every time the functions are called

#0 - GalloDaSballo2022-06-30T01:23:22Z

[Gas-5] Avoid defining DOMAIN_SEPARATOR every time the functions are called

#1 - GalloDaSballo2022-06-30T01:24:37Z

[Gas-3] No need to set 0 on uint variables

[Gas-4] Potential usage of unchecked

[GAS-2] Use != 0 instead of > 0 on uint variables

#2 - GalloDaSballo2022-06-30T01:24:53Z

#0 - GalloDaSballo
2022-07-04T22:26:48Z

[GAS-2] Use `!= 0` instead of `> 0` on uint variables

#0 - GalloDaSballo
2022-06-30T01:23:22Z

#1 - GalloDaSballo
2022-06-30T01:24:37Z

#2 - GalloDaSballo
2022-06-30T01:24:53Z