Velodrome Finance contest - MiloTruck's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L50-L51 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L83-L90

Vulnerability details

Impact

Should a fee-on-transfer token be added as a reward token and deposited, the tokens will be locked in the Bribe contract. Voters will be unable to withdraw their rewards.

Proof of Concept

Tokens are deposited into the Bribe contract using notifyRewardAmount(), where amount of tokens are transferred, then added directly to tokenRewardsPerEpoch[token][adjustedTstamp]:

    _safeTransferFrom(token, msg.sender, address(this), amount);
    tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;

Tokens are transferred out of the Bribe contract using deliverReward(), which attempts to transfer tokenRewardsPerEpoch[token][epochStart] amount of tokens out.

function deliverReward(address token, uint epochStart) external lock returns (uint) {
    require(msg.sender == gauge);
    uint rewardPerEpoch = tokenRewardsPerEpoch[token][epochStart];
    if (rewardPerEpoch > 0) {
        _safeTransfer(token, address(gauge), rewardPerEpoch);
    }
    return rewardPerEpoch;
}

If token happens to be a fee-on-transfer token, deliverReward() will always fail. For example:

• User calls notifyRewardAmount(), with token as token that charges a 2% fee upon any transfer, and amount = 100:
  • _safeTransferFrom() only transfers 98 tokens to the contract due to the 2% fee
  • Assuming epochRewards = 0, tokenRewardsPerEpoch[token][adjustedTstamp] becomes 100
• Later on, when deliverReward() is called with the same token and epochStart:
  • rewardPerEpoch = tokenRewardsPerEpoch[token][epochStart] = 100
  • _safeTransfer attempts to transfer 100 tokens out of the contract
  • However, the contract only contains 98 tokens
  • deliverReward() reverts

The following test, which implements a MockERC20 with fee-on-transfer, demonstrates this:

// Note that the following test was adapted from Bribes.t.sol
function testFailFeeOnTransferToken() public {
    // Deploy ERC20 token with fee-on-transfer
    MockERC20Fee FEE_TOKEN = new MockERC20Fee("FEE", "FEE", 18);

    // Mint FEE token for address(this)
    FEE_TOKEN.mint(address(this), 1e25);
    
    // vote
    VELO.approve(address(escrow), TOKEN_1);
    escrow.create_lock(TOKEN_1, 4 * 365 * 86400);
    vm.warp(block.timestamp + 1);

    address[] memory pools = new address[](1);
    pools[0] = address(pair);
    uint256[] memory weights = new uint256[](1);
    weights[0] = 10000;
    voter.vote(1, pools, weights);

    // and deposit into the gauge!
    pair.approve(address(gauge), 1e9);
    gauge.deposit(1e9, 1);

    vm.warp(block.timestamp + 12 hours); // still prior to epoch start
    vm.roll(block.number + 1);
    assertEq(uint(gauge.getVotingStage(block.timestamp)), uint(Gauge.VotingStage.BribesPhase));

    vm.warp(block.timestamp + 12 hours); // start of epoch
    vm.roll(block.number + 1);
    assertEq(uint(gauge.getVotingStage(block.timestamp)), uint(Gauge.VotingStage.VotesPhase));

    vm.warp(block.timestamp + 5 days); // votes period over
    vm.roll(block.number + 1);

    vm.warp(2 weeks + 1); // emissions start
    vm.roll(block.number + 1);

    minter.update_period();
    distributor.claim(1); // yay this works

    vm.warp(block.timestamp + 1 days); // next votes period start
    vm.roll(block.number + 1);

    // get a bribe
    owner.approve(address(FEE_TOKEN), address(bribe), TOKEN_1);
    bribe.notifyRewardAmount(address(FEE_TOKEN), TOKEN_1);

    vm.warp(block.timestamp + 5 days); // votes period over
    vm.roll(block.number + 1);

    // Atttempt to claim tokens will revert
    voter.distro(); // bribe gets deposited in the gauge
}

Additional Impact

On a larger scale, a malicious attacker could temporarily DOS any Gauge contract. This can be done by:

1. Depositing a fee-on-transfer token into its respective Bribe contract, using notifyRewardAmount(), and adding it as a reward token.
2. This would cause deliverBribes() to fail whenever it is called, thus no one would be able to withdraw any reward tokens from the Gauge contract.

The only way to undo the DOS would be to call swapOutBribeRewardToken() and swap out the fee-on-transfer token for another valid token.

Recommended Mitigation

• The amount of tokens received should be added to epochRewards and stored in tokenRewardsPerEpoch[token][adjustedTstamp], instead of the amount stated for transfer. For example:

    uint256 _before = IERC20(token).balanceOf(address(this));
    _safeTransferFrom(token, msg.sender, address(this), amount);
    uint256 _after = IERC20(token).balanceOf(address(this));

    tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + (_after - _before);

• Alternatively, disallow tokens with fee-on-transfer mechanics to be added as reward tokens.

Gas Optimizations Report

For-Loops: Cache array length outside of loops

Reading an 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.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

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

contracts/Gauge.sol:
 353:        for (uint i = 0; i < tokens.length; i++) {

contracts/RewardsDistributor.sol:
 301:        for (uint i = 0; i < _tokenIds.length; i++) {

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

contracts/Voter.sol:
  76:        for (uint i = 0; i < _tokens.length; i++) {
 266:        for (uint i = 0; i < _gauges.length; i++) {
 304:        for (uint i = 0; i < _gauges.length; i++) {
 310:        for (uint i = 0; i < _gauges.length; i++) {
 346:        for (uint x = 0; x < _gauges.length; x++) {

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++) {
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++) {

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

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

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

can be changed to:

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

Consider making the following change to these lines:

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

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

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/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/Minter.sol:
  57:        for (uint i = 0; i < claimants.length; i++) {

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/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++) {

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

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

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

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

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/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++) {
 199:        user_epoch += 1;
 252:        for (uint i = 0; i < 50; i++) {
 256:        user_epoch += 1;
 301:        for (uint i = 0; i < _tokenIds.length; i++) {

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

contracts/Voter.sol:
  76:        for (uint i = 0; i < _tokens.length; 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/VotingEscrow.sol:
 137:        digits++;
 142:        digits -= 1;
 453:        ownerToNFTokenCount[_to] += 1;
 655:        _epoch += 1;
1076:        attachments[_tokenId] = attachments[_tokenId] + 1;
1081:        attachments[_tokenId] = attachments[_tokenId] - 1;
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++) {

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

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/Pair.sol:
 140:        if (claimed0 > 0 || claimed1 > 0) {
 154:        if (_ratio > 0) {
 164:        if (_ratio > 0) {
 174:        if (_supplied > 0) {
 183:        if (_delta0 > 0) {
 187:        if (_delta1 > 0) {
 207:        if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
 303:        require(liquidity > 0, 'ILM'); // Pair: INSUFFICIENT_LIQUIDITY_MINTED
 322:        require(amount0 > 0 && amount1 > 0, 'ILB'); // Pair: INSUFFICIENT_LIQUIDITY_BURNED
 336:        require(amount0Out > 0 || amount1Out > 0, 'IOA'); // Pair: INSUFFICIENT_OUTPUT_AMOUNT
 345:        if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
 346:        if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
 347:        if (data.length > 0) IPairCallee(to).hook(msg.sender, amount0Out, amount1Out, data); // callback, used for flash loans
 353:        require(amount0In > 0 || amount1In > 0, 'IIA'); // Pair: INSUFFICIENT_INPUT_AMOUNT
 356:        if (amount0In > 0) _update0(amount0In * PairFactory(factory).getFee(stable) / 10000); // accrue fees for token0 and move them out of pool
 357:        if (amount1In > 0) _update1(amount1In * PairFactory(factory).getFee(stable) / 10000); // accrue fees for token1 and move them out of pool
 522:        require(token.code.length > 0);

contracts/Gauge.sol:
 140:        if (claimed0 > 0 || claimed1 > 0) {
 144:        if (_fees0 / DURATION > 0) {
 151:        if (_fees1 / DURATION > 0) {
 182:        if (epochRewards > 0) {
 306:        if (_nCheckPoints > 0 && checkpoints[account][_nCheckPoints - 1].timestamp == _timestamp) {
 309:        bool prevVoteStatus = (_nCheckPoints > 0) ? checkpoints[account][_nCheckPoints].voted : false;
 318:        if (_nCheckPoints > 0 && rewardPerTokenCheckpoints[token][_nCheckPoints - 1].timestamp == timestamp) {
 330:        if (_nCheckPoints > 0 && supplyCheckpoints[_nCheckPoints - 1].timestamp == _timestamp) {
 359:        if (_reward > 0) _safeTransfer(tokens[i], account, _reward);
 407:        if (sp0.supply > 0) {
 447:        if (_endIndex > 0) {
 450:        if (sp0.supply > 0) {
 461:        if (sp.supply > 0) {
 483:        if (_endIndex > 0) {
 512:        require(amount > 0);
 520:        if (tokenId > 0) {
 563:        if (tokenId > 0) {
 592:        require(amount > 0);
 613:        require(rewardRate[token] > 0);
 665:        require(token.code.length > 0);
 672:        require(token.code.length > 0);
 679:        require(token.code.length > 0);

contracts/Bribe.sol:
  42:        require(amount > 0);
  86:        if (rewardPerEpoch > 0) {
  93:        require(token.code.length > 0);
 100:        require(token.code.length > 0);

contracts/Voter.sol:
 111:        if (_votes > 0) {
 167:        if (_usedWeight > 0) IVotingEscrow(_ve).voting(_tokenId);
 227:        if (tokenId > 0) IVotingEscrow(_ve).attach(tokenId);
 239:        if (tokenId > 0) IVotingEscrow(_ve).detach(tokenId);
 259:        if (_ratio > 0) {
 289:        if (_supplied > 0) {
 294:        if (_delta > 0) {
 322:        if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
 352:        require(token.code.length > 0);

contracts/VotingEscrow.sol:
 369:        return size > 0;
 614:        if (_epoch > 0) {
 772:        require(_value > 0); // dev: need non-zero value
 785:        require(_value > 0); // dev: need non-zero value
 819:        assert(_value > 0); // dev: need non-zero value
1214:        if (srcRep != dstRep && _tokenId > 0) {
1217:        uint[] storage srcRepOld = srcRepNum > 0
1237:        uint[] storage dstRepOld = dstRepNum > 0
1269:        _nCheckPoints > 0 &&
1287:        uint[] storage srcRepOld = srcRepNum > 0
1307:        uint[] storage dstRepOld = dstRepNum > 0

contracts/PairFees.sol:
  20:        require(token.code.length > 0);
  29:        if (amount0 > 0) _safeTransfer(token0, recipient, amount0);
  30:        if (amount1 > 0) _safeTransfer(token1, recipient, amount1);

contracts/Router.sol:
  58:        require(amountA > 0, 'Router: INSUFFICIENT_AMOUNT');
  59:        require(reserveA > 0 && reserveB > 0, 'Router: INSUFFICIENT_LIQUIDITY');
 410:        require(token.code.length > 0);
 417:        require(token.code.length > 0);

Arithmetics: Unchecking arithmetic operations that cannot underflow/overflow

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. In some instances, an overflow/underflow is impossible, such as:

• A check is in place before the arithmetic operation
• The value realistically cannot overflow, such as amount of ETH sent

As such, gas can be saved by using an unchecked block to remove the implicit checks:

  unchecked { a += b; }

Below are instances that can be left unchecked:

contracts/Velo.sol:
  43:        balanceOf[_to] += _amount
  44:        totalSupply += _amount;  
  51:        balanceOf[_to] += _value;

contracts/VotingEscrow.sol:
 137:        digits++;
 142:        digits -= 1;
 453:        ownerToNFTokenCount[_to] += 1;
 514:        ownerToNFTokenCount[_from] -= 1;
 734:        _locked.amount += int128(int256(_value));
 789:        ++tokenId;
1076:        attachments[_tokenId] = attachments[_tokenId] + 1;
1081:        attachments[_tokenId] = attachments[_tokenId] - 1;
1148:        votes = votes + _balanceOfNFT(tId, block.timestamp);
1196:        votes = votes + _balanceOfNFT(tId, timestamp);
1232:        numCheckpoints[srcRep] = srcRepNum + 1;
1255:        numCheckpoints[dstRep] = dstRepNum + 1;
1272:        return _nCheckPoints - 1;
1302:        numCheckpoints[srcRep] = srcRepNum + 1;
1330:        numCheckpoints[dstRep] = dstRepNum + 1;

contracts/Pair.sol:
 155:        index0 += _ratio;
 165:        index1 += _ratio;
 185:        claimable0[recipient] += _share;
 189:        claimable1[recipient] += _share;
 258:        priceAverageCumulative += _prices[i];
 277:        nextIndex = i + window;
 282:        index = index + 1;
 447:        totalSupply += amount;
 448:        balanceOf[dst] += amount;
 516:        balanceOf[dst] += amount;

contracts/RewardsDistributor.sol:
 160:        t += WEEK;
 199:        user_epoch += 1;
 213:        week_cursor += WEEK;
 256:        user_epoch += 1;
 270:        week_cursor += WEEK;
 307:        total += amount;

contracts/Gauge.sol:
 141:        uint _fees0 = fees0 + claimed0;
 142:        uint _fees1 = fees1 + claimed1
 214:        return (nCheckpoints - 1);
 223:        uint upper = nCheckpoints - 1;
 246:        return (nCheckpoints - 1);
 255:        uint upper = nCheckpoints - 1;
 287:        uint upper = nCheckpoints - 1;
 311:        numCheckpoints[account] = _nCheckPoints + 1;
 322:        rewardPerTokenNumCheckpoints[token] = _nCheckPoints + 1;
 334:        supplyNumCheckpoints = _nCheckPoints + 1;
 368:        derivedSupply += _derivedBalance;
 410:        reward += _reward;
 445:        uint _endIndex = supplyNumCheckpoints-1;
 453:        reward += _reward;
 463:        reward += _reward;
 517:        totalSupply += amount;
 518:        balanceOf[msg.sender] += amount;
 535:        derivedSupply += _derivedBalance;
 575:        derivedSupply += _derivedBalance;
 586:        uint _remaining = periodFinish[token] - block.timestamp;
 607:        uint _remaining = periodFinish[token] - block.timestamp;
 652:        uint _remaining = periodFinish[token] - block.timestamp;
 659:        periodFinish[token] = epochStart + DURATION;

contracts/Bribe.sol:
  37:        uint bribeEnd = bribeStart + DURATION - COOLDOWN;

contracts/Voter.sol:
 112:        _totalWeight += _votes;
 144:        _totalVoteWeight += _weights[i];
 159:        weights[_pool] += _poolWeight;
 160:        votes[_tokenId][_pool] += _poolWeight;
 161:        _usedWeight += _poolWeight;
 162:        _totalWeight += _poolWeight;
 168:        totalWeight += uint256(_totalWeight);
 260:        index += _ratio;
 269:        claimable[_gauge] += _share;

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/Minter.sol:
  30:        uint public constant MAX_TEAM_RATE = 50;

contracts/VeloGovernor.sol:
  19:        uint256 public constant MAX_PROPOSAL_NUMERATOR = 50;
  20:        uint256 public constant PROPOSAL_DENOMINATOR = 1000;

contracts/VotingEscrow.sol:
1106:        bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
1109:        bytes32 public constant DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");
1113:        uint public constant MAX_DELEGATES = 1024;

contracts/factories/PairFactory.sol:
  14:        uint256 public constant MAX_FEE = 5;

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/Gauge.sol:
 163:        function getVotingStage(uint timestamp) public pure returns (VotingStage) {

contracts/Voter.sol:
  82:        function setGovernor(address _governor) public {
  87:        function setEmergencyCouncil(address _council) public {
 178:        function whitelist(address _token) public {

contracts/VotingEscrow.sol:
1184:        function getPastVotes(address account, uint timestamp)
1185:            public
1186:            view
1187:            returns (uint)
1188:        {

1349:        function delegate(address delegatee) public {

1354:        function delegateBySig(
1355:            address delegatee,
1356:            uint nonce,
1357:            uint expiry,
1358:            uint8 v,
1359:            bytes32 r,
1360:            bytes32 s
1361:        ) public {

contracts/redeem/RedemptionReceiver.sol:
 107:        function addressFromPackedBytes(bytes memory toAddressBytes)
 108:            public
 109:            pure
 110:            returns (address toAddress)
 111:        {

contracts/redeem/RedemptionSender.sol:
  28:        function redeemWEVE(
  29:            uint256 amount,
  30:            address zroPaymentAddress,
  31:            bytes memory zroTransactionParams
  32:        ) public payable {

contracts/factories/PairFactory.sol:
  76:        function getFee(bool _stable) public view returns(uint256) {

Errors: Reduce the length 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.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/VotingEscrow.sol:
1378:        require(
1379:            signatory != address(0),
1380:            "VotingEscrow::delegateBySig: invalid signature"
1381:        );

1382:        require(
1383:            nonce == nonces[signatory]++,
1384:            "VotingEscrow::delegateBySig: invalid nonce"
1385:        );

1386:        require(
1387:            block.timestamp <= expiry,
1388:            "VotingEscrow::delegateBySig: signature expired"
1389:        );

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

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 can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/Gauge.sol:
 594:        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
 615:        require(rewardRate[token] <= balance / DURATION, "Provided reward too high");
 642:        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");

contracts/Minter.sol:
  65:        require(msg.sender == team, "not team");
  70:        require(msg.sender == pendingTeam, "not pending team");
  75:        require(msg.sender == team, "not team");
  76:        require(_teamRate <= MAX_TEAM_RATE, "rate too high");

contracts/VeloGovernor.sol:
  40:        require(msg.sender == team, "not team");
  45:        require(msg.sender == team, "not team");
  46:        require(numerator <= MAX_PROPOSAL_NUMERATOR, "numerator too high");

contracts/Bribe.sol:
  31:        require(gauge == address(0), "gauge already set");
  44:        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
  69:        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");

contracts/Voter.sol:
 190:        require(gauges[_pool] == address(0x0), "exists");
 192:        require(IPairFactory(factory).isPair(_pool), "!_pool");
 194:        require(isWhitelisted[tokenA] && isWhitelisted[tokenB], "!whitelisted");
 211:        require(msg.sender == emergencyCouncil, "not emergency council");
 212:        require(isAlive[_gauge], "gauge already dead");
 218:        require(msg.sender == emergencyCouncil, "not emergency council");
 219:        require(!isAlive[_gauge], "gauge already alive");
 318:        require((dayCalc < BRIBE_LAG) || (dayCalc > (DURATION + BRIBE_LAG)), "cannot claim during votes period");

contracts/VotingEscrow.sol:
 163:        require(idToOwner[_tokenId] != address(0), "Query for nonexistent token");
 307:        require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");
 518:        require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");
 846:        require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");
 849:        require(block.timestamp >= _locked.end, "The lock didn't expire");
1085:        require(attachments[_from] == 0 && !voted[_from], "attached");

contracts/redeem/RedemptionReceiver.sol:
  24:        require(_fantomChainId == 12 || _fantomChainId == 10012, "CHAIN_ID_NOT_FTM");
  43:        require(msg.sender == deployer, "ONLY_DEPLOYER");
  44:        require(fantomSender == address(0), "ALREADY_INITIALIZED");
  78:        require(fantomSender != address(0), "NOT_INITIALIZED");

contracts/redeem/RedemptionSender.sol:
  21:        require(_optimismChainId == 11 || _optimismChainId == 10011, "CHAIN_ID_NOT_OP");
  33:        require(amount != 0, "AMOUNT_ZERO");

contracts/governance/L2Governor.sol:
  68:        require(_msgSender() == _executor(), "Governor: onlyGovernance");
 260:        require(targets.length == values.length, "Governor: invalid proposal length");
 261:        require(targets.length == calldatas.length, "Governor: invalid proposal length");
 262:        require(targets.length > 0, "Governor: empty proposal");
 265:        require(proposal.voteStart.isUnset(), "Governor: proposal already exists");
 525:        require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active");

contracts/governance/L2GovernorCountingSimple.sol:
  96:        require(!proposalvote.hasVoted[account], "GovernorVotingSimple: vote already cast");

Unecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/Pair.sol:
  20:        uint public totalSupply = 0;
  61:        uint public index0 = 0;
  62:        uint public index1 = 0;
 273:        uint nextIndex = 0;
 274:        uint index = 0;

contracts/Gauge.sol:
 222:        uint lower = 0;
 254:        uint lower = 0;
 286:        uint lower = 0;
 481:        uint reward = 0;
 551:        uint tokenId = 0;

contracts/RewardsDistributor.sol:
  73:        uint next_week = 0;
 103:        uint _min = 0;
 119:        uint _min = 0;
 170:        uint user_epoch = 0;
 171:        uint to_distribute = 0;
 227:        uint user_epoch = 0;
 228:        uint to_distribute = 0;
 299:        uint total = 0;

contracts/Voter.sol:
 101:        uint256 _totalWeight = 0;
 139:        uint256 _totalVoteWeight = 0;
 140:        uint256 _totalWeight = 0;
 141:        uint256 _usedWeight = 0;

contracts/VotingEscrow.sol:
 622:        uint block_slope = 0; // dblock/dt
 884:        uint _min = 0;
 940:        uint _min = 0;
 960:        uint d_block = 0;
 961:        uint d_t = 0;
 996:        uint dt = 0;
1145:        uint votes = 0;
1168:        uint32 lower = 0;
1192:        uint votes = 0;

contracts/Router.sol:
 112:        uint _totalSupply = 0;

contracts/Velo.sol:
   9:        uint public totalSupply = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/Pair.sol:
 175:        uint _supplyIndex0 = supplyIndex0[recipient];
 176:        uint _supplyIndex1 = supplyIndex1[recipient];
 313:        (uint _reserve0, uint _reserve1) = (reserve0, reserve1);
 413:        (uint _reserve0, uint _reserve1) = (reserve0, reserve1);

contracts/Gauge.sol:
 586:        uint _remaining = periodFinish[token] - block.timestamp;
 607:        uint _remaining = periodFinish[token] - block.timestamp;
 652:        uint _remaining = periodFinish[token] - block.timestamp;

contracts/Bribe.sol:
  48:        uint epochRewards = tokenRewardsPerEpoch[token][adjustedTstamp];

contracts/Voter.sol:
 287:        address _pool = poolForGauge[_gauge];
 290:        uint _supplyIndex = supplyIndex[_gauge];

contracts/VotingEscrow.sol:
 246:        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);
 247:        bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
 285:        bool spenderIsOwner = owner == _spender;
 286:        bool spenderIsApproved = _spender == idToApprovals[_tokenId];
 287:        bool spenderIsApprovedForAll = (ownerToOperators[owner])[_spender];
 554:        uint uepoch = user_point_epoch[_tokenId];
1047:        uint _epoch = epoch;
1147:        uint tId = _tokenIds[i];
1194:        uint tId = _tokenIds[i];
1265:        uint _timestamp = block.timestamp;

contracts/Velo.sol:
  61:        uint allowed_from = allowance[_from][msg.sender];

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

Consider declaring these variables as immutable:

contracts/Pair.sol:
  13:        string public name;
  14:        string public symbol;

contracts/Gauge.sol:
  20:        bool public isForPair;

contracts/RewardsDistributor.sol:
  32:        uint public start_time;
  40:        address public voting_escrow;
  41:        address public token;

Variables declared as constant are expressions, not constants

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.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

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

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

contracts/RewardsDistributor.sol:
  30:        uint constant WEEK = 7 * 86400;

contracts/Minter.sol:
  14:        uint internal constant WEEK = 86400 * 7;
  24:        uint internal constant LOCK = 86400 * 7 * 52 * 4;

contracts/VotingEscrow.sol:
 542:        uint internal constant MAXTIME = 4 * 365 * 86400;
 543:        int128 internal constant iMAXTIME = 4 * 365 * 86400;
1106:        bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
1109:        bytes32 public constant DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");

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

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.

approve() can be optimized in VotingEscrow.sol

In the approve() function, idToOwner[_tokenId] is fetched from storage and assigned to owner as shown:

contracts/VotingEscrow.sol:
 240:        address owner = idToOwner[_tokenId];

Thus, to prevent fetching values from storage unnecessarily, the code below:

contracts/VotingEscrow.sol:
 246:        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);

should be changed to:

contracts/VotingEscrow.sol:
 246:        bool senderIsOwner = (owner == msg.sender);

Changes in gas cost

• Deployment size: 3163 to 2991
• Average runtime cost: 15237 to 15065

Unnecessary declaration of variables in _deposit_for() of VotingEscrow.sol

In _deposit_for(), the following variables are unneeded:

• Line 727: locked_balance can be used directly, instead of declaring _locked
• Line 746: msg.sender can be used instead of declaring _from

As such, the function can be rewritten to:

function _deposit_for(
    uint _tokenId,
    uint _value,
    uint unlock_time,
    LockedBalance memory locked_balance,
    DepositType deposit_type
) internal {
    // LockedBalance memory _locked = locked_balance;
    uint supply_before = supply;

    supply = supply_before + _value;
    LockedBalance memory old_locked;
    (old_locked.amount, old_locked.end) = (locked_balance.amount, locked_balance.end);
    // Adding to existing lock, or if a lock is expired - creating a new one
    locked_balance.amount += int128(int256(_value));
    if (unlock_time != 0) {
        locked_balance.end = unlock_time;
    }
    locked[_tokenId] = locked_balance;

    // Possibilities:
    // Both old_locked.end could be current or expired (>/< block.timestamp)
    // value == 0 (extend lock) or value > 0 (add to lock or extend lock)
    // _locked.end > block.timestamp (always)
    _checkpoint(_tokenId, old_locked, locked_balance);

    // address from = msg.sender;
    if (_value != 0 && deposit_type != DepositType.MERGE_TYPE) {
        assert(IERC20(token).transferFrom(msg.sender, address(this), _value));
    }

    emit Deposit(msg.sender, _tokenId, _value, locked_balance.end, deposit_type, block.timestamp);
    emit Supply(supply_before, supply_before + _value);
}

Changes in gas cost

Note that this was measured using deposit_for(), which internally calls _deposit_for()

• Deployment size: 150941 to 150921
• Average runtime cost: 151226 to 151206

Parameter validation checks should be first to save gas upon revert

Parameter validation checks should be completed before any other code. This helps to save runtime gas costs should the function revert.

In VotingEscrow.sol, the functions deposit_for(), _create_lock() and increase_amount() validate the parameter _value:

function deposit_for(uint _tokenId, uint _value) external nonreentrant {
    LockedBalance memory _locked = locked[_tokenId];

    require(_value > 0); // dev: need non-zero value
    // ...
}

This check should be done before any other code in the function to save gas costs, as follows:

function deposit_for(uint _tokenId, uint _value) external nonreentrant {
    require(_value > 0); // dev: need non-zero value

    LockedBalance memory _locked = locked[_tokenId];
    // ...
}

The same applies for _create_lock() and increase_amount() as well.