Velodrome Finance contest - Dravee's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L50 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L516 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L604 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L610 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Router.sol#L399 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L748

Vulnerability details

As arbitrary ERC20 tokens can be passed, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Affected code:

contracts/contracts/Bribe.sol:
   50:       _safeTransferFrom(token, msg.sender, address(this), amount); //@audit med: incompatible with FoT tokens
   51        tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;

contracts/contracts/Gauge.sol:
  516          _safeTransferFrom(stake, msg.sender, address(this), amount); //@audit med: incompatible with FoT tokens
  517:         totalSupply += amount;
  518:         balanceOf[msg.sender] += amount;

  603          if (block.timestamp >= periodFinish[token]) {
  604              _safeTransferFrom(token, msg.sender, address(this), amount); //@audit med: incompatible with FoT tokens
  605:             rewardRate[token] = amount / DURATION;
  606          } else {
  607              uint _remaining = periodFinish[token] - block.timestamp;
  608              uint _left = _remaining * rewardRate[token];
  609              require(amount > _left);
  610              _safeTransferFrom(token, msg.sender, address(this), amount); //@audit med: incompatible with FoT tokens
  611:             rewardRate[token] = (amount + _left) / DURATION; 
  612          }

contracts/contracts/Router.sol:
  399          _safeTransferFrom(routes[0].from, msg.sender, pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]);//@audit med: incompatible with FoT tokens
  400:         _swap(amounts, routes, to); 

contracts/contracts/Voter.sol:
  257          _safeTransferFrom(base, msg.sender, address(this), amount); // transfer the distro in //@audit med: incompatible with FoT tokens
  258:         uint256 _ratio = amount * 1e18 / totalWeight; // 1e18 adjustment is removed during claim 

contracts/contracts/VotingEscrow.sol:
  747          if (_value != 0 && deposit_type != DepositType.MERGE_TYPE) {
  748              assert(IERC20(token).transferFrom(from, address(this), _value));//@audit med: incompatible with FoT tokens
  749          }
  750  
  751:         emit Deposit(from, _tokenId, _value, _locked.end, deposit_type, block.timestamp);
  752          emit Supply(supply_before, supply_before + _value);

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

Table of Contents:

• [L-01] Check Effects Interactions pattern not respected
• [L-02] Deprecated approve() function
• [L-03] No account existence check for low-level call
• [L-04] Add a timelock and event to critical functions
• [L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
• [N-01] Open TODOS
• [N-02] Missing friendly revert strings
• [N-03] require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

[L-01] Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

Affected code:

File: RewardsDistributor.sol
282:     function claim(uint _tokenId) external returns (uint) {
283:         if (block.timestamp >= time_cursor) _checkpoint_total_supply();
284:         uint _last_token_time = last_token_time;
285:         _last_token_time = _last_token_time / WEEK * WEEK;
286:         uint amount = _claim(_tokenId, voting_escrow, _last_token_time);
287:         if (amount != 0) {
288:             IVotingEscrow(voting_escrow).deposit_for(_tokenId, amount); //@audit external function call
289:             token_last_balance -= amount; //@audit low: CEIP not respected here
290:         }
291:         return amount;
292:     }

[L-02] Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack and failing for certain token implementations that do not return a boolean value. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

contracts/contracts/Gauge.sol:
  681:         token.call(abi.encodeWithSelector(IERC20.approve.selector, spender, value));

contracts/contracts/Minter.sol:
   56:         _velo.approve(address(_ve), type(uint).max);
  133:             _velo.approve(address(_voter), weekly);

contracts/contracts/RewardsDistributor.sol:
  57:         IERC20(_token).approve(_voting_escrow, type(uint).max);

contracts/contracts/Voter.sol:
  198:         IERC20(base).approve(_gauge, type(uint).max);

[L-03] No account existence check for low-level call

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling.

Consider checking for account-existence before the call() to make this safely extendable to user-controlled address contexts in future (or, at least, prevent the address(0) entry):

• contracts/contracts/Router.sol:

230:         if (msg.value > amountETH) _safeTransferETH(msg.sender, msg.value - amountETH);
...
274:         _safeTransferETH(to, amountETH);
...
390:         _safeTransferETH(to, amounts[amounts.length - 1]);
...
404:     function _safeTransferETH(address to, uint value) internal {
405:         (bool success,) = to.call{value:value}(new bytes(0)); //@audit : "to" can be address(0)
406:         require(success, 'TransferHelper: ETH_TRANSFER_FAILED');
407:     }

[L-04] Add a timelock and event to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock for feeManager and an event for PairFactory.setFee():

File: PairFactory.sol
65:     function setFee(bool _stable, uint256 _fee) external {
66:         require(msg.sender == feeManager, 'not fee manager');
67:         require(_fee <= MAX_FEE, 'fee too high');
68:         require(_fee != 0, 'fee must be nonzero');
69:         if (_stable) {
70:             stableFee = _fee;
71:         } else {
72:             volatileFee = _fee;
73:         }
74:     }

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

• File: Pair.sol

477:         bytes32 digest = keccak256(
478:             abi.encodePacked(
479:                 '\x19\x01',
480:                 DOMAIN_SEPARATOR,
481:                 keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline))
482:             )
483:         );

• File: Router.sol

48:         pair = address(uint160(uint256(keccak256(abi.encodePacked(
49:             hex'ff',
50:             factory,
51:             keccak256(abi.encodePacked(token0, token1, stable)),
52:             pairCodeHash // init code hash
53:         )))));

• File: VotingEscrow.sol

1374:         bytes32 digest = keccak256(
1375:             abi.encodePacked("\x19\x01", domainSeparator, structHash)
1376:         );

• File: PairFactory.sol

93:         bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters

[N-01] Open TODOS

Consider resolving the TODOs before deploying.

Minter.sol:11:// TODO: decide on whether to abstract from VELO or not. currently it's only somewhat abstracted (e.g. L38)
VelodromeLibrary.sol:9:    IRouter internal immutable router; // TODO make modifiable?
VotingEscrow.sol:314:        // TODO delegates
VotingEscrow.sol:465:        // TODO add delegates
VotingEscrow.sol:524:        // TODO add delegates

[N-02] Missing friendly revert strings

Friendly messages should exist for users to understand what went wrong:

contracts/contracts/Bribe.sol:
   24:       require(_unlocked == 1);
   42:       require(amount > 0);
   67:     require(msg.sender == gauge);
   76:     require(msg.sender == gauge);
   77:     require(rewards[i] == oldToken);
   84:     require(msg.sender == gauge);
   93:       require(token.code.length > 0);
   96:       require(success && (data.length == 0 || abi.decode(data, (bool))));
  100:       require(token.code.length > 0);
  103:       require(success && (data.length == 0 || abi.decode(data, (bool))));

contracts/contracts/Gauge.sol:
  125:         require(_unlocked == 1);
  174:         require(msg.sender == voter);
  189:         require(msg.sender == voter);
  348:         require(msg.sender == account || msg.sender == voter);
  512:         require(amount > 0);
  521:             require(IVotingEscrow(_ve).ownerOf(tokenId) == msg.sender);
  526:             require(tokenIds[msg.sender] == tokenId);
  564:             require(tokenId == tokenIds[msg.sender]);
  591:         require(token != stake);
  592:         require(amount > 0);
  609:             require(amount > _left);
  613:         require(rewardRate[token] > 0);
  628:         require(rewards[i] == oldToken);
  640:         require(msg.sender == bribe);
  654:             require(amount > _left);
  665:         require(token.code.length > 0);
  668:         require(success && (data.length == 0 || abi.decode(data, (bool))));
  672:         require(token.code.length > 0);
  675:         require(success && (data.length == 0 || abi.decode(data, (bool))));
  679:         require(token.code.length > 0);
  682:         require(success && (data.length == 0 || abi.decode(data, (bool))));

contracts/contracts/Minter.sol:
   54:         require(initializer == msg.sender);
  128:             require(_velo.transfer(team, _teamEmissions));
  129:             require(_velo.transfer(address(_rewards_distributor), _growth));

contracts/contracts/Pair.sol:
  111:         require(_unlocked == 1);
  335:         require(!PairFactory(factory).isPaused());
  522:         require(token.code.length > 0);
  525:         require(success && (data.length == 0 || abi.decode(data, (bool))));

contracts/contracts/PairFees.sol:
  20:         require(token.code.length > 0);
  23:         require(success && (data.length == 0 || abi.decode(data, (bool))));
  28:         require(msg.sender == pair);

contracts/contracts/RewardsDistributor.sol:
   98:         assert(msg.sender == depositor);
  319:         require(msg.sender == depositor);

contracts/contracts/Router.sol:
   36:         assert(msg.sender == address(weth)); // only accept ETH via fallback from the WETH contract
  164:         require(amountADesired >= amountAMin);
  165:         require(amountBDesired >= amountBMin);
  181:                 assert(amountAOptimal <= amountADesired);
  227:         assert(weth.transfer(pair, amountETH));
  245:         require(IPair(pair).transferFrom(msg.sender, pair, liquidity)); // send liquidity to pair
  373:         assert(weth.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));
  410:         require(token.code.length > 0);
  413:         require(success && (data.length == 0 || abi.decode(data, (bool))));
  417:         require(token.code.length > 0);
  420:         require(success && (data.length == 0 || abi.decode(data, (bool))));

contracts/contracts/Velo.sol:
  27:         require(msg.sender == minter);
  32:         require(msg.sender == minter);
  69:         require(msg.sender == minter);
  75:         require(msg.sender == redemptionReceiver);

contracts/contracts/Voter.sol:
   68:         require(_unlocked == 1);
   75:         require(msg.sender == minter);
   83:         require(msg.sender == governor);
   88:         require(msg.sender == emergencyCouncil);
   93:         require(IVotingEscrow(_ve).isApprovedOrOwner(msg.sender, _tokenId));
  153:                 require(votes[_tokenId][_pool] == 0);
  154:                 require(_poolWeight != 0);
  173:         require(IVotingEscrow(_ve).isApprovedOrOwner(msg.sender, tokenId));
  174:         require(_poolVote.length == _weights.length);
  179:         require(msg.sender == governor);
  184:         require(!isWhitelisted[_token]);
  225:         require(isGauge[msg.sender]);
  226:         require(isAlive[msg.sender]); // killed gauges cannot attach tokens to themselves
  232:         require(isGauge[msg.sender]);
  233:         require(isAlive[msg.sender]);
  238:         require(isGauge[msg.sender]);
  244:         require(isGauge[msg.sender]);
  286:         require(isAlive[_gauge]); // killed gauges cannot be updated
  316:         require(isAlive[_gauge]); // killed gauges cannot distribute
  352:         require(token.code.length > 0);
  355:         require(success && (data.length == 0 || abi.decode(data, (bool))));

contracts/contracts/VotingEscrow.sol:
   112:         require(_entered_state == _not_entered);
   242:         require(owner != address(0));
   244:         require(_approved != owner);
   248:         require(senderIsOwner || senderIsApprovedForAll);
   262:         assert(_operator != msg.sender);
   272:         assert(idToOwner[_tokenId] == _owner);
   309:         require(_isApprovedOrOwner(_sender, _tokenId));
   447:         assert(idToOwner[_tokenId] == address(0));
   464:         assert(_to != address(0));
   508:         assert(idToOwner[_tokenId] == _from);
   748:             assert(IERC20(token).transferFrom(from, address(this), _value));
   772:         require(_value > 0); // dev: need non-zero value
   785:         require(_value > 0); // dev: need non-zero value
   815:         assert(_isApprovedOrOwner(msg.sender, _tokenId));
   819:         assert(_value > 0); // dev: need non-zero value
   829:         assert(_isApprovedOrOwner(msg.sender, _tokenId));
   845:         assert(_isApprovedOrOwner(msg.sender, _tokenId));
   861:         assert(IERC20(token).transfer(msg.sender, value));
   937:         assert(_block <= block.number);
   991:         assert(_block <= block.number);
  1060:         require(msg.sender == voter);
  1065:         require(msg.sender == voter);
  1070:         require(msg.sender == voter);
  1075:         require(msg.sender == voter);
  1080:         require(msg.sender == voter);
  1086:         require(_from != _to);
  1087:         require(_isApprovedOrOwner(msg.sender, _from));
  1088:         require(_isApprovedOrOwner(msg.sender, _to));

contracts/contracts/factories/GaugeFactory.sol:
  18:         require(msg.sender == team);

contracts/contracts/factories/PairFactory.sol:
  41:         require(msg.sender == pauser);
  46:         require(msg.sender == pendingPauser);
  51:         require(msg.sender == pauser);

[N-03] require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

RewardsDistributor.sol:98:        assert(msg.sender == depositor);
Router.sol:36:        assert(msg.sender == address(weth)); // only accept ETH via fallback from the WETH contract
Router.sol:181:                assert(amountAOptimal <= amountADesired);
Router.sol:227:        assert(weth.transfer(pair, amountETH));
Router.sol:373:        assert(weth.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));
VotingEscrow.sol:262:        assert(_operator != msg.sender);
VotingEscrow.sol:272:        assert(idToOwner[_tokenId] == _owner);
VotingEscrow.sol:447:        assert(idToOwner[_tokenId] == address(0));
VotingEscrow.sol:464:        assert(_to != address(0));
VotingEscrow.sol:508:        assert(idToOwner[_tokenId] == _from);
VotingEscrow.sol:748:            assert(IERC20(token).transferFrom(from, address(this), _value));
VotingEscrow.sol:815:        assert(_isApprovedOrOwner(msg.sender, _tokenId));
VotingEscrow.sol:819:        assert(_value > 0); // dev: need non-zero value
VotingEscrow.sol:829:        assert(_isApprovedOrOwner(msg.sender, _tokenId));
VotingEscrow.sol:845:        assert(_isApprovedOrOwner(msg.sender, _tokenId));
VotingEscrow.sol:861:        assert(IERC20(token).transfer(msg.sender, value));
VotingEscrow.sol:937:        assert(_block <= block.number);
VotingEscrow.sol:991:        assert(_block <= block.number);

As the Solidity version is > 0.8.* the remaining gas would still be refunded in case of failure.

Table of Contents:

• Cheap Contract Deployment Through Clones
• Bytes constants are more efficient than string constants
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• >= is cheaper than > (and <= cheaper than <)
• Use shift right/left instead of division/multiplication if possible
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• Increments can be unchecked
• No need to explicitly initialize variables with default values
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas

Cheap Contract Deployment Through Clones

See @audit tag:

contracts/contracts/Pair.sol:
   93:         fees = address(new PairFees(_token0, _token1));

contracts/contracts/factories/BribeFactory.sol:
  10:         last_gauge = address(new Bribe());

contracts/contracts/factories/GaugeFactory.sol:
  24:         last_gauge = address(new Gauge(_pool, _bribe, _ve, msg.sender, isPair));
  30:         last_gauge = address(new Gauge(_pool, _bribe, _ve, _voter, isPair));

contracts/contracts/factories/PairFactory.sol:
  95:         pair = address(new Pair{salt:salt}());

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

I suggest applying a similar pattern (use the cloneDeterministic method to mimic the current create2 in PairFactory.sol)

Bytes constants are more efficient than string constants

From the Solidity doc:

If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

Why do Solidity examples use bytes32 type instead of string?

bytes32 uses less gas because it fits in a single word of the EVM, and string is a dynamically sized-type which has current limitations in Solidity (such as can't be returned from a function to a contract).

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Instances of string constant that can be replaced by bytes(1..32) constant :

Velo.sol:6:    string public constant name = "Velodrome";
Velo.sol:7:    string public constant symbol = "VELO";
VotingEscrow.sol:122:    string constant public name = "veNFT";
VotingEscrow.sol:123:    string constant public symbol = "veNFT";
VotingEscrow.sol:124:    string constant public version = "1.0.0";

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

Bribe.sol:42:      require(amount > 0);
Bribe.sol:93:      require(token.code.length > 0);
Bribe.sol:100:      require(token.code.length > 0);
Gauge.sol:512:        require(amount > 0);
Gauge.sol:592:        require(amount > 0);
Gauge.sol:613:        require(rewardRate[token] > 0);
Gauge.sol:665:        require(token.code.length > 0);
Gauge.sol:672:        require(token.code.length > 0);
Gauge.sol:679:        require(token.code.length > 0);
Pair.sol:303:        require(liquidity > 0, 'ILM'); // Pair: INSUFFICIENT_LIQUIDITY_MINTED
Pair.sol:322:        require(amount0 > 0 && amount1 > 0, 'ILB'); // Pair: INSUFFICIENT_LIQUIDITY_BURNED
Pair.sol:336:        require(amount0Out > 0 || amount1Out > 0, 'IOA'); // Pair: INSUFFICIENT_OUTPUT_AMOUNT
Pair.sol:353:        require(amount0In > 0 || amount1In > 0, 'IIA'); // Pair: INSUFFICIENT_INPUT_AMOUNT
Pair.sol:361:        require(_k(_balance0, _balance1) >= _k(_reserve0, _reserve1), 'K'); // Pair: K
Pair.sol:522:        require(token.code.length > 0);
PairFees.sol:20:        require(token.code.length > 0);
Router.sol:58:        require(amountA > 0, 'Router: INSUFFICIENT_AMOUNT');
Router.sol:59:        require(reserveA > 0 && reserveB > 0, 'Router: INSUFFICIENT_LIQUIDITY');
Router.sol:410:        require(token.code.length > 0);
Router.sol:417:        require(token.code.length > 0);
Voter.sol:352:        require(token.code.length > 0);
VotingEscrow.sol:772:        require(_value > 0); // dev: need non-zero value
VotingEscrow.sol:773:        require(_locked.amount > 0, 'No existing lock found');
VotingEscrow.sol:785:        require(_value > 0); // dev: need non-zero value
VotingEscrow.sol:819:        assert(_value > 0); // dev: need non-zero value
VotingEscrow.sol:820:        require(_locked.amount > 0, 'No existing lock found');
VotingEscrow.sol:835:        require(_locked.amount > 0, 'Nothing is locked');

Also, please enable the Optimizer.

>= is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Consider replacing strict inequalities with non-strict ones to save some gas here:

factories/PairFactory.sol:90:        (address token0, address token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
Pair.sol:351:        uint amount0In = _balance0 > _reserve0 - amount0Out ? _balance0 - (_reserve0 - amount0Out) : 0;
Pair.sol:352:        uint amount1In = _balance1 > _reserve1 - amount1Out ? _balance1 - (_reserve1 - amount1Out) : 0;
Router.sol:41:        (token0, token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);

Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated.

• Use >> 1 instead of / 2
• Use >> 2 instead of / 4
• Use << 3 instead of * 8

Affected code:

Gauge.sol:225:            uint center = upper - (upper - lower) / 2; // ceil, avoiding overflow
Gauge.sol:257:            uint center = upper - (upper - lower) / 2; // ceil, avoiding overflow
Gauge.sol:289:            uint center = upper - (upper - lower) / 2; // ceil, avoiding overflow
RewardsDistributor.sol:107:            uint _mid = (_min + _max + 2) / 2;
RewardsDistributor.sol:123:            uint _mid = (_min + _max + 2) / 2;
VotingEscrow.sol:891:            uint _mid = (_min + _max + 1) / 2;
VotingEscrow.sol:947:            uint _mid = (_min + _max + 1) / 2;
VotingEscrow.sol:1171:            uint32 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:

Gauge.sol:353:        for (uint i = 0; i < tokens.length; i++) {
Minter.sol:57:        for (uint i = 0; i < claimants.length; i++) {
Pair.sol:257:        for (uint i = 0; i < _prices.length; i++) {
RewardsDistributor.sol:301:        for (uint i = 0; i < _tokenIds.length; i++) {
Router.sol:90:        for (uint i = 0; i < routes.length; i++) {
Router.sol:316:        for (uint i = 0; i < routes.length; i++) {
Voter.sol:76:        for (uint i = 0; i < _tokens.length; i++) {
Voter.sol:266:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:304:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:310:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:346:        for (uint x = 0; x < _gauges.length; x++) {
VotingEscrow.sol:1146:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1193:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1225:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1249:                for (uint i = 0; i < dstRepOld.length; i++) {
VotingEscrow.sol:1295:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1320:                for (uint i = 0; i < dstRepOld.length; i++) {

++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

Gauge.sol:179:        for (uint i = 0; i < numRewards; i++) {
Gauge.sol:353:        for (uint i = 0; i < tokens.length; i++) {
Gauge.sol:405:        for (uint i = _startIndex; i < _endIndex; i++) {
Gauge.sol:426:        for (uint i; i < length; i++) {
Gauge.sol:448:            for (uint i = _startIndex; i < _endIndex; i++) {
Gauge.sol:484:            for (uint i = _startIndex; i < _endIndex; i++) {
Minter.sol:57:        for (uint i = 0; i < claimants.length; i++) {
Pair.sol:257:        for (uint i = 0; i < _prices.length; i++) {
Pair.sol:389:        for (uint i = 0; i < 255; i++) {
RewardsDistributor.sol:75:        for (uint i = 0; i < 20; i++) {
RewardsDistributor.sol:105:        for (uint i = 0; i < 128; i++) {
RewardsDistributor.sol:121:        for (uint i = 0; i < 128; i++) {
RewardsDistributor.sol:148:        for (uint i = 0; i < 20; i++) {
RewardsDistributor.sol:195:        for (uint i = 0; i < 50; i++) {
RewardsDistributor.sol:199:                user_epoch += 1;
RewardsDistributor.sol:252:        for (uint i = 0; i < 50; i++) {
RewardsDistributor.sol:256:                user_epoch += 1;
RewardsDistributor.sol:301:        for (uint i = 0; i < _tokenIds.length; i++) {
Router.sol:90:        for (uint i = 0; i < routes.length; i++) {
Router.sol:316:        for (uint i = 0; i < routes.length; i++) {
VelodromeLibrary.sol:24:        for (uint i = 0; i < 255; i++) {
Voter.sol:76:        for (uint i = 0; i < _tokens.length; i++) {
Voter.sol:103:        for (uint i = 0; i < _poolVoteCnt; i ++) {
Voter.sol:128:        for (uint i = 0; i < _poolCnt; i ++) {
Voter.sol:143:        for (uint i = 0; i < _poolCnt; i++) {
Voter.sol:147:        for (uint i = 0; i < _poolCnt; i++) {
Voter.sol:266:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:272:        for (uint i = start; i < end; i++) {
Voter.sol:304:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:310:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:340:        for (uint x = start; x < finish; x++) {
Voter.sol:346:        for (uint x = 0; x < _gauges.length; x++) {
VotingEscrow.sol:137:            digits++;
VotingEscrow.sol:142:            digits -= 1;
VotingEscrow.sol:453:        ownerToNFTokenCount[_to] += 1;
VotingEscrow.sol:514:        ownerToNFTokenCount[_from] -= 1;
VotingEscrow.sol:655:                _epoch += 1;
VotingEscrow.sol:1146:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1193:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1225:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1249:                for (uint i = 0; i < dstRepOld.length; i++) {
VotingEscrow.sol:1295:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1320:                for (uint i = 0; i < dstRepOld.length; i++) {
VotingEscrow.sol:1325:                for (uint i = 0; i < ownerTokenCount; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Affected code:

contracts/contracts/Gauge.sol:
  179:         for (uint i = 0; i < numRewards; i++) {
  353:         for (uint i = 0; i < tokens.length; i++) {
  405:         for (uint i = _startIndex; i < _endIndex; i++) {
  426:         for (uint i; i < length; i++) {
  448:             for (uint i = _startIndex; i < _endIndex; i++) {
  484:             for (uint i = _startIndex; i < _endIndex; i++) {

contracts/contracts/Minter.sol:
  57:         for (uint i = 0; i < claimants.length; i++) {

contracts/contracts/Pair.sol:
  257:         for (uint i = 0; i < _prices.length; i++) {
  389:         for (uint i = 0; i < 255; i++) {

contracts/contracts/RewardsDistributor.sol:
   75:         for (uint i = 0; i < 20; i++) {
  105:         for (uint i = 0; i < 128; i++) {
  121:         for (uint i = 0; i < 128; i++) {
  148:         for (uint i = 0; i < 20; i++) {
  195:         for (uint i = 0; i < 50; i++) {
  252:         for (uint i = 0; i < 50; i++) {
  301:         for (uint i = 0; i < _tokenIds.length; i++) {

contracts/contracts/Router.sol:
   90:         for (uint i = 0; i < routes.length; i++) {
  316:         for (uint i = 0; i < routes.length; i++) {

contracts/contracts/VelodromeLibrary.sol:
  24:         for (uint i = 0; i < 255; i++) {

contracts/contracts/Voter.sol:
   76:         for (uint i = 0; i < _tokens.length; i++) {
  103:         for (uint i = 0; i < _poolVoteCnt; i ++) {
  128:         for (uint i = 0; i < _poolCnt; i ++) {
  143:         for (uint i = 0; i < _poolCnt; i++) {
  147:         for (uint i = 0; i < _poolCnt; i++) {
  266:         for (uint i = 0; i < _gauges.length; i++) {
  272:         for (uint i = start; i < end; i++) {
  304:         for (uint i = 0; i < _gauges.length; i++) {
  310:         for (uint i = 0; i < _gauges.length; i++) {
  340:         for (uint x = start; x < finish; x++) {
  346:         for (uint x = 0; x < _gauges.length; x++) {

contracts/contracts/VotingEscrow.sol:
   632:             for (uint i = 0; i < 255; ++i) {
   886:         for (uint i = 0; i < 128; ++i) {
   942:         for (uint i = 0; i < 128; ++i) {
  1017:         for (uint i = 0; i < 255; ++i) {
  1146:         for (uint i = 0; i < _tokenIds.length; i++) {
  1193:         for (uint i = 0; i < _tokenIds.length; i++) {
  1225:                 for (uint i = 0; i < srcRepOld.length; i++) {
  1249:                 for (uint i = 0; i < dstRepOld.length; i++) {
  1295:                 for (uint i = 0; i < srcRepOld.length; i++) {
  1320:                 for (uint i = 0; i < dstRepOld.length; i++) {
  1325:                 for (uint i = 0; i < ownerTokenCount; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  //or i--
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  //or unchecked { --i; }
}  

The risk of overflow is inexistant for uint256 here.

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

Gauge.sol:179:        for (uint i = 0; i < numRewards; i++) {
Gauge.sol:222:        uint lower = 0;
Gauge.sol:254:        uint lower = 0;
Gauge.sol:286:        uint lower = 0;
Gauge.sol:353:        for (uint i = 0; i < tokens.length; i++) {
Gauge.sol:481:        uint reward = 0;
Gauge.sol:551:        uint tokenId = 0;
Minter.sol:57:        for (uint i = 0; i < claimants.length; i++) {
Pair.sol:20:    uint public totalSupply = 0;
Pair.sol:61:    uint public index0 = 0;
Pair.sol:62:    uint public index1 = 0;
Pair.sol:257:        for (uint i = 0; i < _prices.length; i++) {
Pair.sol:273:        uint nextIndex = 0;
Pair.sol:274:        uint index = 0;
Pair.sol:389:        for (uint i = 0; i < 255; i++) {
RewardsDistributor.sol:73:        uint next_week = 0;
RewardsDistributor.sol:75:        for (uint i = 0; i < 20; i++) {
RewardsDistributor.sol:103:        uint _min = 0;
RewardsDistributor.sol:105:        for (uint i = 0; i < 128; i++) {
RewardsDistributor.sol:119:        uint _min = 0;
RewardsDistributor.sol:121:        for (uint i = 0; i < 128; i++) {
RewardsDistributor.sol:148:        for (uint i = 0; i < 20; i++) {
RewardsDistributor.sol:154:                int128 dt = 0;
RewardsDistributor.sol:170:        uint user_epoch = 0;
RewardsDistributor.sol:171:        uint to_distribute = 0;
RewardsDistributor.sol:195:        for (uint i = 0; i < 50; i++) {
RewardsDistributor.sol:227:        uint user_epoch = 0;
RewardsDistributor.sol:228:        uint to_distribute = 0;
RewardsDistributor.sol:252:        for (uint i = 0; i < 50; i++) {
RewardsDistributor.sol:299:        uint total = 0;
RewardsDistributor.sol:301:        for (uint i = 0; i < _tokenIds.length; i++) {
Router.sol:90:        for (uint i = 0; i < routes.length; i++) {
Router.sol:112:        uint _totalSupply = 0;
Router.sol:316:        for (uint i = 0; i < routes.length; i++) {
Velo.sol:9:    uint public totalSupply = 0;
VelodromeLibrary.sol:24:        for (uint i = 0; i < 255; i++) {
Voter.sol:76:        for (uint i = 0; i < _tokens.length; i++) {
Voter.sol:101:        uint256 _totalWeight = 0;
Voter.sol:103:        for (uint i = 0; i < _poolVoteCnt; i ++) {
Voter.sol:128:        for (uint i = 0; i < _poolCnt; i ++) {
Voter.sol:139:        uint256 _totalVoteWeight = 0;
Voter.sol:140:        uint256 _totalWeight = 0;
Voter.sol:141:        uint256 _usedWeight = 0;
Voter.sol:143:        for (uint i = 0; i < _poolCnt; i++) {
Voter.sol:147:        for (uint i = 0; i < _poolCnt; i++) {
Voter.sol:266:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:304:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:310:        for (uint i = 0; i < _gauges.length; i++) {
Voter.sol:346:        for (uint x = 0; x < _gauges.length; x++) {
VotingEscrow.sol:584:        int128 old_dslope = 0;
VotingEscrow.sol:585:        int128 new_dslope = 0;
VotingEscrow.sol:622:        uint block_slope = 0; // dblock/dt
VotingEscrow.sol:632:            for (uint i = 0; i < 255; ++i) {
VotingEscrow.sol:636:                int128 d_slope = 0;
VotingEscrow.sol:884:        uint _min = 0;
VotingEscrow.sol:886:        for (uint i = 0; i < 128; ++i) {
VotingEscrow.sol:940:        uint _min = 0;
VotingEscrow.sol:942:        for (uint i = 0; i < 128; ++i) {
VotingEscrow.sol:960:        uint d_block = 0;
VotingEscrow.sol:961:        uint d_t = 0;
VotingEscrow.sol:996:        uint dt = 0;
VotingEscrow.sol:1017:        for (uint i = 0; i < 255; ++i) {
VotingEscrow.sol:1019:            int128 d_slope = 0;
VotingEscrow.sol:1145:        uint votes = 0;
VotingEscrow.sol:1146:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1168:        uint32 lower = 0;
VotingEscrow.sol:1192:        uint votes = 0;
VotingEscrow.sol:1193:        for (uint i = 0; i < _tokenIds.length; i++) {
VotingEscrow.sol:1225:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1249:                for (uint i = 0; i < dstRepOld.length; i++) {
VotingEscrow.sol:1295:                for (uint i = 0; i < srcRepOld.length; i++) {
VotingEscrow.sol:1320:                for (uint i = 0; i < dstRepOld.length; i++) {
VotingEscrow.sol:1325:                for (uint i = 0; i < ownerTokenCount; i++) {

I suggest removing explicit initializations for default values.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

VotingEscrow.sol:1247:                    "dstRep would have too many tokenIds"
VotingEscrow.sol:1317:                    "dstRep would have too many tokenIds"
VotingEscrow.sol:1380:            "VotingEscrow::delegateBySig: invalid signature"
VotingEscrow.sol:1384:            "VotingEscrow::delegateBySig: invalid nonce"
VotingEscrow.sol:1388:            "VotingEscrow::delegateBySig: signature expired" 

I suggest shortening the revert strings to fit in 32 bytes.

Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

I suggest replacing all revert strings with custom errors in the solution. Impacted files:

• factories/GaugeFactory.sol
• factories/PairFactory.sol
• redeem/RedemptionReceiver.sol
• redeem/RedemptionSender.sol
• Bribe.sol
• Gauge.sol
• Minter.sol
• Pair.sol
• PairFees.sol
• RewardsDistributor.sol
• Router.sol
• Velo.sol
• VeloGovernor.sol
• Voter.sol
• VotingEscrow.sol