Velodrome Finance contest - oyc_109's results

No Transfer Ownership Pattern

description

The current minter transfer process involves the current minter calling setMinter(). If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, or address(0), breaking all functions that requre msg.sender == _minter

Recommend considering implementing a two step process where the minter nominates an account and the nominated account needs to call an accept() function for the transfer of minter to fully succeed. This ensures the nominated EOA account is a valid and active account.

findings

/2022-05-velodrome/contracts/contracts/Velo.sol
26:     function setMinter(address _minter) external {
27:         require(msg.sender == minter);
28:         minter = _minter;
29:     }

the setVoter function in VotingEscrow also does not implement this pattern

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
1059:     function setVoter(address _voter) external {
1060:         require(msg.sender == voter);
1061:         voter = _voter;
1062:     }

open TODO

description

clean up any open TODOs in the comments

findings

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
314: // TODO delegates
465: // TODO add delegates
524: // TODO add delegates

/2022-05-velodrome/contracts/contracts/VelodromeLibrary.sol
9: IRouter internal immutable router; // TODO make modifiable?

redundant function call

description

calling mint of 0 tokens for msg.sender wastes gass

findings

/2022-05-velodrome/contracts/contracts/Velo.sol
22: _mint(msg.sender, 0);

Long Revert Strings

description

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
398: revert('ERC721: transfer to non ERC721Receiver implementer');
1317: "dstRep would have too many tokenIds"
1380: "VotingEscrow::delegateBySig: invalid signature"
1384: "VotingEscrow::delegateBySig: invalid nonce"
1388: "VotingEscrow::delegateBySig: signature expired"

/2022-05-velodrome/contracts/contracts/Router.sol
341: require(amounts[amounts.length - 1] >= amountOutMin, 'Router: INSUFFICIENT_OUTPUT_AMOUNT');

do not calculate constants

description

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

Consequences: each usage of a "constant" costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

findings

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
542:     uint internal constant MAXTIME = 4 * 365 * 86400;
543:     int128 internal constant iMAXTIME = 4 * 365 * 86400;

/2022-05-velodrome/contracts/contracts/Pair.sol
30: uint internal constant MINIMUM_LIQUIDITY = 10**3;

/2022-05-velodrome/contracts/contracts/Router.sol
21: uint internal constant MINIMUM_LIQUIDITY = 10**3;

/2022-05-velodrome/contracts/contracts/RewardsDistributor.sol
30: uint constant WEEK = 7 * 86400;

/2022-05-velodrome/contracts/contracts/Minter.sol
14: uint internal constant WEEK = 86400 * 7; // allows minting once per week (reset every Thursday 00:00 UTC)
24: uint internal constant LOCK = 86400 * 7 * 52 * 4;

/2022-05-velodrome/contracts/contracts/Gauge.sol
36: uint internal constant PRECISION = 10 ** 18;

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/2022-05-velodrome/contracts/contracts/Velo.sol
9: uint public totalSupply = 0;

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
584:         int128 old_dslope = 0;
585:         int128 new_dslope = 0;
622: uint block_slope = 0; // dblock/dt
632: for (uint i = 0; i < 255; ++i) {
636: int128 d_slope = 0;
884: uint _min = 0;
886: for (uint i = 0; i < 128; ++i) {
940: uint _min = 0;
942: for (uint i = 0; i < 128; ++i) {
960:         uint d_block = 0;
961:         uint d_t = 0;
996: uint dt = 0;
1017: for (uint i = 0; i < 255; ++i) {
1019: int128 d_slope = 0;
1168: uint32 lower = 0;

/2022-05-velodrome/contracts/contracts/Pair.sol
20: uint public totalSupply = 0;
61:     uint public index0 = 0;
62:     uint public index1 = 0;
63: 

/2022-05-velodrome/contracts/contracts/Pair.sol
273:         uint nextIndex = 0;
274:         uint index = 0;

/2022-05-velodrome/contracts/contracts/RewardsDistributor.sol
73: uint next_week = 0;
103: uint _min = 0;
170:         uint user_epoch = 0;
171:         uint to_distribute = 0;
299: uint total = 0;

/2022-05-velodrome/contracts/contracts/Gauge.sol
222: uint lower = 0;

for loop optimisations

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

Suggest not initializing the for loop counter to 0.

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

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Suggest storing the array’s length in a variable before the for-loop, and use it instead:

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Suggest using ++i instead of i++ to increment the value of an uint variable.

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.

taking all of the above, the recommended format for gas savings is

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

findings

/2022-05-velodrome/contracts/contracts/VotingEscrow.sol
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++) {
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++) {

/2022-05-velodrome/contracts/contracts/Pair.sol
257: for (uint i = 0; i < _prices.length; i++) {

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

/2022-05-velodrome/contracts/contracts/RewardsDistributor.sol
75: for (uint i = 0; i < 20; i++) {
105: for (uint i = 0; i < 128; i++) {
195: for (uint i = 0; i < 50; i++) {

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

/2022-05-velodrome/contracts/contracts/Gauge.sol
179: for (uint i = 0; i < numRewards; i++) {

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

findings

/2022-05-velodrome/contracts/contracts/Router.sol
315: function _swap(uint[] memory amounts, route[] memory routes, address _to) internal virtual {

/2022-05-velodrome/contracts/contracts/Minter.sol
50:         address[] memory claimants,
51:         uint[] memory amounts,

use shift operator

description

Gas can be saved by using >> operator instead of dividing by 2

use >> operator

findings

/2022-05-velodrome/contracts/contracts/RewardsDistributor.sol
107: uint _mid = (_min + _max + 2) / 2;
123: uint _mid = (_min + _max + 2) / 2;