Reserve contest - Aymen0909's results

QA Report

Summary

Findings

1- delegateBySig doesn't revert when signer is address(0) :

Risk : Low

The function delegateBySig from the StRSRVotes contract is used to allow the users to delegate by signatures and it uses the ECDSAUpgradeable.recover function to get the signer address, this function can in some cases return address(0) but the delegateBySig does not check the signer address and thus the function will not revert in case of signer == address(0).

Proof of Concept

the isssue occurs in the delegateBySig function below :

File: contracts/p1/StRSRVotes.sol Line 118-135

function delegateBySig(
    address delegatee,
    uint256 nonce,
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) public {
    require(block.timestamp <= expiry, "ERC20Votes: signature expired");
    address signer = ECDSAUpgradeable.recover(
        _hashTypedDataV4(keccak256(abi.encode(_DELEGATE_TYPEHASH, delegatee, nonce, expiry))),
        v,
        r,
        s
    );
    require(nonce == _useNonce(signer), "ERC20Votes: invalid nonce");
    _delegate(signer, delegatee);
}

As you can see the returned signer address is used without checking its value and thus the call will not revert if its equla to address(0).

Mitigation

Add a non zero address check on the returned signer address

2- require() checks on inputs should be placed first :

require() checks statement that are used to check the input values should be placed at the beginnning of the functions to avoid unecessary runtime.

Risk : NON CRITICAL

Proof of Concept

There are 2 instances of this issue :

File: contracts/p1/StRSR.sol Line 812-817

function setUnstakingDelay(uint48 val) public governance {
    require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
    emit UnstakingDelaySet(unstakingDelay, val);
    unstakingDelay = val;
    require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
}

These function can be rewritten as follow :

function setUnstakingDelay(uint48 val) public governance {
    require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
    require(rewardPeriod * 2 <= val, "unstakingDelay/rewardPeriod incompatible");
    emit UnstakingDelaySet(unstakingDelay, val);
    unstakingDelay = val;
}

File: contracts/p1/StRSR.sol Line 820-825

function setRewardPeriod(uint48 val) public governance {
    require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");
    emit RewardPeriodSet(rewardPeriod, val);
    rewardPeriod = val;
    require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
}

The function can be rewritten as follow :

function setRewardPeriod(uint48 val) public governance {
    require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");
    require(val * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    emit RewardPeriodSet(rewardPeriod, val);
    rewardPeriod = val;
}

3- Duplicated require() checks should be refactored to a modifier :

require() checks statement used multiple times inside a contract should be refactored to a modifier for better readability.

Risk : NON CRITICAL

Proof of Concept

There are 4 instances of this issue in AssetRegistry:

File: contracts/p1/AssetRegistry.sol

74      require(_erc20s.contains(address(asset.erc20())), "no ERC20 collision");
88      require(_erc20s.contains(address(asset.erc20())), "no asset to unregister");
103     require(_erc20s.contains(address(erc20)), "erc20 unregistered");
111     require(_erc20s.contains(address(erc20)), "erc20 unregistered");

Those checks should be replaced by a registred modifier as follow :

modifier registred(address erc20){
    require(_erc20s.contains(erc20), "erc20 unregistered");
    _;
}

File: contracts/p1/StRSRVotes.sol

77      require(blockNumber < block.number, "ERC20Votes: block not yet mined");
83      require(blockNumber < block.number, "ERC20Votes: block not yet mined");
89      require(blockNumber < block.number, "ERC20Votes: block not yet mined");

Those checks should be replaced by a isMinted modifier as follow :

modifier isMinted(uint256 blockNumber){
    require(blockNumber < block.number, "ERC20Votes: block not yet mined");
    _;
}

4- Use solidity time-based units :

Solidity contains time-based units (seconds, minutes, hours, days and weeks) which should be used when declaring time based variables/constants instead of using literal numbers, this is done for better readability and to avoid mistakes.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/p1/BackingManager.sol Line 33

uint48 public constant MAX_TRADING_DELAY = 31536000; // {s} 1 year

File: contracts/p1/Broker.sol Line 24

uint48 public constant MAX_AUCTION_LENGTH = 604800; // {s} max valid duration - 1 week

File: contracts/p1/Furnace.sol Line 16

uint48 public constant MAX_PERIOD = 31536000; // {s} 1 year

File: contracts/p1/StRSR.sol Line 37-38

uint48 public constant MAX_UNSTAKING_DELAY = 31536000; // {s} 1 year
uint48 public constant MAX_REWARD_PERIOD = 31536000; // {s} 1 year

Mitigation

Use time units when declaring time-based variables.

Gas Optimizations

Summary

Findings

1- Usage of uint/int smaller than 32 bytes (256 bits) cost more gas :

State variable declared as uint/int smaller than 32 bytes (256 bits) will cost more gas when used in the code logic as the compiler must first convert them back to uint256 before using them, the only case where using smaller uint/int size matter is when you store many state variables in the same storage slot otherwise it's better to declare them as uint256 to save gas in functions calls.

There are 5 instances of this issue :

File: contracts/p1/RToken.sol Line 50

uint192 public issuanceRate;

File: contracts/p1/RToken.sol Line 67-76

uint192 public basketsNeeded; // D18{BU}

// ==== Slow Issuance State====

// When all pending issuances will have vested.
uint192 private allVestAt; // D18{fractional block number}

// Enforce a fixed issuanceRate throughout the entire block by caching it.
// Both of these MUST only be modified by whenFinished()
uint192 private lastIssRate; // D18{rTok/block}

File: contracts/p1/StRSR.sol Line 147

uint48 public payoutLastPaid;

2- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 4 instances of this issue :

File: contracts/p1/StRSR.sol Line 270

uint256 newStakeRSR = (FIX_ONE_256 * totalStakes + (stakeRate - 1)) / stakeRate; 

In the code linked above the value of stakeRate is read multiple times (2) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: contracts/p1/StRSR.sol Line 324

uint256 newDraftRSR = (newTotalDrafts * FIX_ONE_256 + (draftRate - 1)) / draftRate;

In the code linked above the value of draftRate is read multiple times (2) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: contracts/p1/StRSR.sol Line 391-398

if (stakeRSR > 0) {
    // Downcast is safe: totalStakes is 1e38 at most so expression maximum value is 1e56
    stakeRate = uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR);
}
if (stakeRSR == 0 || stakeRate > MAX_STAKE_RATE) {
    seizedRSR += stakeRSR;
    beginEra();
}

In the code linked above the value of stakeRSR is read multiple times (5) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: contracts/p1/StRSR.sol Line 406-414

if (draftRSR > 0) {
    // Downcast is safe: totalDrafts is 1e38 at most so expression maximum value is 1e56
    draftRate = uint192((FIX_ONE_256 * totalDrafts + (draftRSR - 1)) / draftRSR);
}

if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) {
    seizedRSR += draftRSR;
    beginDraftEra();
}

In the code linked above the value of draftRSR is read multiple times (5) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

3- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 3 instances of this issue:

File: contracts/libraries/RedemptionBattery.sol Line 47

battery.lastCharge = charge - amount;

The operation in the code above cannot underflow because of the if check that precede it Line 44 so it should be marked as unchecked.

File: contracts/p1/StRSR.sol Line 698-699

stakes[era][account] += amount;
totalStakes += amount;

The operations in the code above cannot overflow because of the if check that precedes them Line 696 and because we always have totalStakes >= stakes[era][account] so they should be marked as unchecked.

4- Remove redundant checks :

The following require check in the unstake function is redundant because the _burn function is also called by unstake and it already contains the same check on the account staking balance, so this check is redundant and should be removed to save gas.

File: contracts/p1/StRSR.sol Line 260

require(stakes[era][account] >= stakeAmount, "Not enough balance");

5- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 9 instances of this issue:

File: contracts/p1/Furnace.sol

81      lastPayout += numPeriods * period;

File: contracts/p1/StRSR.sol

229     stakeRSR += rsrAmount;
387     stakeRSR -= stakeRSRToTake;
513     stakeRSR += payout;
516     payoutLastPaid += numPeriods * rewardPeriod;
549     draftRSR += rsrAmount;
698     stakes[era][account] += amount;
699     totalStakes += amount;
721     totalStakes -= amount;

6- Splitting require() statements that uses && saves gas :

There are 10 instances of this issue :

File: contracts/p1/Broker.sol

134     require(
            newAuctionLength > 0 && newAuctionLength <= MAX_AUCTION_LENGTH,
            "invalid auctionLength"
        );

File: contracts/p1/Furnace.sol

89      require(period_ > 0 && period_ <= MAX_PERIOD, "invalid period");

File: contracts/p1/RevenueTrader.sol

72      require(buyPrice > 0 && buyPrice < FIX_MAX, "buy asset price unknown");

File: contracts/p1/RToken.sol

410     require(queue.left <= endId && endId <= queue.right, "out of range");
590     require(val > 0 && val <= MAX_ISSUANCE_RATE, "invalid issuanceRate");
623     require(index >= item.left && index < item.right, "out of range");     
741     require(queue.left <= endId && endId <= queue.right, "out of range");
813     require(uint192(low) >= 1e9 && uint192(high) <= 1e27, "BU rate out of range");

File: contracts/p1/StRSR.sol

813     require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
821     require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");

7- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 38 instances of this issue:

File: contracts/p1/AssetRegistry.sol

38      for (uint256 i = 0; i < length; ++i)
49      for (uint256 i = 0; i < length; ++i)
127     for (uint256 i = 0; i < length; ++i)
138     for (uint256 i = 0; i < length; ++i)

File: contracts/p1/BackingManager.sol

221     for (uint256 i = 0; i < length; ++i)
238     for (uint256 i = 0; i < length; ++i)

File: contracts/p1/BasketHandler.sol

70      for (uint256 i = 0; i < length; ++i)
78      for (uint256 i = 0; i < length; ++i)
218     for (uint256 i = 0; i < config.erc20s.length; ++i)
227     for (uint256 i = 0; i < erc20s.length; ++i)
262     for (uint256 i = 0; i < erc20s.length; ++i)
286     for (uint256 i = 0; i < size; ++i)
337     for (uint256 i = 0; i < len; ++i)
397     for (uint256 i = 0; i < len; ++i)
416     for (uint256 i = 0; i < length; ++i) 
437     for (uint256 i = 0; i < length; ++i)
530     for (uint256 i = 0; i < basketLength; ++i)
548     for (uint256 i = 0; i < basketLength; ++i)
586     for (uint256 i = 0; i < targetsLength; ++i)
643     for (uint256 i = 0; i < basketLength; ++i)
653     for (uint256 i = 0; i < erc20s.length; i++)
707     for (uint256 i = 0; i < erc20s.length; i++)
725     for (uint256 i = 0; i < erc20s.length; i++)

File: contracts/p1/Distributor.sol

108     for (uint256 i = 0; i < destinations.length(); ++i) 
133     for (uint256 i = 0; i < numTransfers; i++) 
143     for (uint256 i = 0; i < length; ++i)

File: contracts/p1/RToken.sol

270     for (uint256 i = 0; i < erc20s.length; ++i)
303     for (uint256 i = 0; i < basketSize; ++i)
329     for (uint256 i = 0; i < basketSize; ++i)
334     for (uint256 i = 0; i < basketSize; ++i)
478     for (uint256 i = 0; i < erc20length; ++i)
501     for (uint256 i = 0; i < erc20length; ++i)
647     for (uint256 i = 0; i < tokensLen; ++i)
683     for (uint256 i = 0; i < tokensLen; ++i)
711     for (uint256 i = 0; i < queue.tokens.length; ++i)
757     for (uint256 i = 0; i < tokensLen; ++i)
767     for (uint256 i = 0; i < tokensLen; ++i)
793     for (uint256 i = 0; i < tokensLen; ++i)