Reserve contest - delfin454000's results

Summary

Low risk findings

Non-critical findings

Low risk findings

Below is an example of a valid use of assert in Reserve Protocol:

GnosisTrade.sol: L176-183

        // Optionally process settlement of the auction in Gnosis
        if (!isAuctionCleared()) {
            // By design, we don't rely on this return value at all, just the
            // "cleared" state of the auction, and the token balances this contract owns.
            // slither-disable-next-line unused-return
            gnosis.settleAuction(auctionId);
            assert(isAuctionCleared());
        }

The following assert functions appear not to meet the criteria and should be replaced by require functions:

BackingManager.sol: L249

BasketHandler.sol: L556

RecollateralizationLib.sol: L110

RewardableLib.sol: L78

RewardableLib.sol: L102

TradeLib.sol: L44

TradeLib.sol: L108-113

TradeLib.sol: L170

Trading.sol: L114

StaticATokenLM.sol: L345

GnosisTrade.sol: L98

---

---

Non-critical findings

---

TradeLib.sol: L38-41

    function prepareTradeSell(TradeInfo memory trade, TradingRules memory rules)
        internal
        view
        returns (bool notDust, TradeRequest memory req)
  }

Here notDust is never used, which is especially odd since it is discussed fairly extensively in the comments above the function.

Similarly for the function below, in which notDust is also named:

TradeLib.sol: L103-106

---

IFacadeAct.sol: L18

    function getActCalldata(RTokenP1 rToken) external returns (address to, bytes memory calldata_);

calldata_ is never used

---

IGnosis.sol: L45

    function settleAuction(uint256 auctionId) external returns (bytes32 encodedOrder);

encodedOrder is never used (it is used in a mock file)

---

IRToken.sol: L119

    function issue(address recipient, uint256 amount) external returns (uint256 mintedAmount);

mintedAmount is not used (it is used in the out of scope contracts/p0/RToken.sol)

---

---

StRSR.sol: L126-129

    bytes32 private constant _PERMIT_TYPEHASH =
        keccak256(
            "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
        );

---

StRSRVotes.sol: L27-28

    bytes32 private constant _DELEGATE_TYPEHASH =
        keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");

---

Fixed.sol: L33

bytes32 constant UIntOutofBoundsHash = keccak256(abi.encodeWithSignature("UIntOutOfBounds()"));

---

StaticATokenLM.sol: L41-44

    bytes32 internal constant EIP712_DOMAIN =
        keccak256(
            "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
        );

---

StaticATokenLM.sol: L45-48

    bytes32 public constant PERMIT_TYPEHASH =
        keccak256(
            "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
        );

---

StaticATokenLM.sol: L49-52

    bytes32 public constant METADEPOSIT_TYPEHASH =
        keccak256(
            "Deposit(address depositor,address recipient,uint256 value,uint16 referralCode,bool fromUnderlying,uint256 nonce,uint256 deadline)"
        );

---

StaticATokenLM.sol: L53-56

    bytes32 public constant METAWITHDRAWAL_TYPEHASH =
        keccak256(
            "Withdraw(address owner,address recipient,uint256 staticAmount,uint256 dynamicAmount,bool toUnderlying,uint256 nonce,uint256 deadline)"
        );

---

ERC20PermitUpgradeable.sol: L39-42

    bytes32 private constant _PERMIT_TYPEHASH =
        keccak256(
            "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
        );

---

---

Trading.sol: L66

    function settleTrade(IERC20 sell) external notPausedOrFrozen nonReentrant {

---

---

Main.sol: L64

    function _authorizeUpgrade(address newImplementation) internal override onlyRole(OWNER) {}

Similarly for the following function:

Component.sol: L57

---

---

Broker.sol: L96

        // In the future we'll have more sophisticated choice logic here, probably by trade size

---

Main.sol: L67-68

     * @dev This empty reserved space is put in place to allow future versions to add new
     * variables without shifting down storage in the inheritance chain.

---

IFacadeAct.sol: L16

    /// @dev Don't actually execute this!

---

---

IDistributor.sol: L28

    event DistributionSet(address dest, uint16 rTokenDist, uint16 rsrDist);

---

IRToken.sol: L78

    event Redemption(address indexed redeemer, uint256 indexed amount, uint192 baskets);

---

IRToken.sol: L83

    event BasketsNeededChanged(uint192 oldBasketsNeeded, uint192 newBasketsNeeded);

---

IRToken.sol: L87

    event Melted(uint256 amount);

---

IAToken.sol: L15

    event Mint(address indexed from, uint256 value, uint256 index);

---

IAToken.sol: L37

    event Burn(address indexed from, address indexed target, uint256 value, uint256 index);

---

IAToken.sol: L46

    event BalanceTransfer(address indexed from, address indexed to, uint256 value, uint256 index);

---

IAaveIncentivesController.sol: L6

    event RewardsAccrued(address indexed user, uint256 amount);

---

IAaveIncentivesController.sol: L8

    event RewardsClaimed(address indexed user, address indexed to, uint256 amount);

---

IBasketHandler.sol: L28

    event BasketSet(uint256 indexed nonce, IERC20[] erc20s, uint192[] refAmts, bool disabled);

In this case, the only change needed is to convert bool disabled to bool indexed disabled since IERC20[] and uint192[] are complex data types

---

---

IAaveIncentivesController.sol: L3

pragma experimental ABIEncoderV2;

---

Similarly for the following:

IStaticATokenLM.sol: L3

RayMathNoRounding.sol: L3

StaticATokenLM.sol: L3

---

The majority of the contracts use pragma solidity 0.8.9. Below are the contracts with even earlier versions. All need to be upgraded before implementation.

pragma solidity ^0.6.0

ERC20.sol: L3

pragma solidity >=0.6.0 <0.8.0

ReentrancyGuard.sol: L3

pragma solidity 0.6.12

IAToken.sol: L2

IAaveIncentivesController.sol: L2

IStaticATokenLM.sol: L2

RayMathNoRounding.sol: L2

StaticATokenErrors.sol: L2

StaticATokenLM.sol: L2

---

---

Main.sol: L64

Component.sol: L57

Fixed.sol: L116

Fixed.sol: L229

Fixed.sol: L243

Fixed.sol: L352

Fixed.sol: L359-363

Governance.sol: L145-149

ERC20PermitUpgradeable.sol: L67

---

---

IAaveIncentivesController.sol: L90-91

     * @dev Claims reward for an user on behalf, on all the assets of the lending pool, accumulating the pending rewards. The caller must
     * be whitelisted via "allowClaimOnBehalf" function by the RewardsAdmin role manager

Suggestion:

     * @dev Claims reward for an user on behalf, on all the assets of the lending pool, accumulating the pending rewards —
     *   the caller must be whitelisted via "allowClaimOnBehalf" function by the RewardsAdmin role manager.

---

StaticATokenLM.sol: L520

     * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet unclaimed) rewards in RAY (rounded down).

Suggestion:

     * @notice Compute the pending in RAY (rounded down). Pending is the amount
     *   to add (not yet unclaimed) rewards in RAY (rounded down).

---

---

BackingManager.sol: L89

    /// Mointain the overall backing policy; handout assets otherwise

Change Mointain to Maintain

---

BasketHandler.sol: L321

    /// lowLow should be nonzero when the asset might be worth selling

Change lowLow to lotLow

---

BasketHandler.sol: L537

        // As such, they're each interepreted as a map from target name -> target weight

Change interepreted to interpreted

---

The same typo (permisionlessly) occurs in both lines below:

Furnace.sol: L10

IFurnace.sol: L9

 * @notice A helper to melt RTokens slowly and permisionlessly.

Change permisionlessly to permissionlessly in both cases

---

Furnace.sol: L66

    //   lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay leriod)

Change leriod to period

---

RToken.sol: L109

    // issuances, and so any particular issuance is actually the _difference_ between two adjaacent

Change adjaacent to adjacent

---

RToken.sol: L112

    // The way to keep an IssueQueue striaght in your head is to think of each TotalIssue item as a

Change striaght to straight

---

StRSR.sol: L25

 *   If this happens, users balances are zereod out and StRSR is re-issued at a 1:1 exchange rate

Change zereod to zeroed

---

StRSR.sol: L541

    //   r'.queue is r.queue with a new entry appeneded for (totalDrafts' - totalDraft) drafts

Change appeneded to appended

---

StRSR.sol: L570

    /// Overriden in StRSRVotes to handle rebases

Change Overriden to Overridden

---

The same typo (that it's) occurs in both lines below:

RecollateralizationLib.sol: L306

RecollateralizationLib.sol: L311

    //   and sellAmount is the quantity of that token that it's in surplus (in qTok).

Change that it's to that's in both cases

---

FacadeRead.sol: L201

    /// @dev Indices are shared aross return values

Change aross to across

---

FacadeRead.sol: L256

        //      this functions will then panic when `uoaHeld.div(uoaNeeded)`

Change functions to function

---

IAsset.sol: L86

    /// VERY IMPORTANT: In any valid implemntation, status() MUST become DISABLED in refresh() if

Change implemntation to implementation

---

The same typo (repeated word the) occurs in both lines below:

IBackingManager.sol: L17

IAToken.sol: L22

 *   vast majority of cases we expect the the O(n^2) function to be acceptable.

Change the the to the in both cases

---

IFacadeRead.sol: L100

    /// @return backing The worst-case collaterazation % the protocol will have after done trading

Change collaterazation to collateralization

---

IFacadeWrite.sol: L68

 * @notice A UX-friendly layer for interactin with the protocol

Change interactin to interacting

---

IRToken.sol: L74

    /// @param redeemer The address of the account redeeeming RTokens

Change redeeeming to redeeming

---

IRToken.sol: L178

    /// Set the fraction of the RToken supply that can be reedemed at once

Change reedemed to redeemed

---

IStRSR.sol: L71

    /// Emitted whenever RSR are paids out

Change paids to paid

---

IStRSR.sol: L76

    /// Emitted if all the RSR in the unstakin pool is seized, and all ongoing unstaking is voided.

Change unstakin to unstaking

---

Auth.sol: L22

     * Typically freezing thaws on its own in a predetemined number of blocks.

Change predetemined to predetermined

---

StaticATokenLM.sol: L524

     * @return The amound of pending rewards in RAY

Change amound to amount

---

FiatCollateral.sol: L21

    uint48 delayUntilDefault; // {s} The number of seconds an oracle can mulfunction

Change mulfunction to malfunction

---

FiatCollateral.sol: L45

    //                In this case, the asset may recover, reachiving _whenDefault == NEVER.

Change reachiving to reaching if that was what was intended

---

GnosisTrade.sol: L47

    // from this trade's acution will all eventually go to origin.

Change acution to auction

---

GnosisTrade.sol: L121

        uint96 minBuyAmount = uint96(Math.max(1, req.minBuyAmount)); // Safe downcast; require'd

Change require'd to required

---

GnosisTrade.sol: L166

    //   transfer all balancess of `buy` and `sell` at this address to `origin`

Change balancess to balances

---

---

IAaveIncentivesController.sol: L41

     * @dev Returns the whitelisted claimer for a certain address (0x0 if not set)

Suggestion: Change whitelisted to allowlisted

---

Similarly for the following instances of whitelist variants:

IAaveIncentivesController.sol: L34

IAaveIncentivesController.sol: L90-91

---

---

Summary

Gas optimizations

---

---

Broker.sol: L134-137

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

Recommendation:

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

---

Deployer.sol: L48-65

        require(
            address(rsr_) != address(0) &&
                address(gnosis_) != address(0) &&
                address(rsrAsset_) != address(0) &&
                address(implementations_.main) != address(0) &&
                address(implementations_.trade) != address(0) &&
                address(implementations_.components.assetRegistry) != address(0) &&
                address(implementations_.components.backingManager) != address(0) &&
                address(implementations_.components.basketHandler) != address(0) &&
                address(implementations_.components.broker) != address(0) &&
                address(implementations_.components.distributor) != address(0) &&
                address(implementations_.components.furnace) != address(0) &&
                address(implementations_.components.rsrTrader) != address(0) &&
                address(implementations_.components.rTokenTrader) != address(0) &&
                address(implementations_.components.rToken) != address(0) &&
                address(implementations_.components.stRSR) != address(0),
            "invalid address"
        );

Recommendation:

        require(address(rsr_) != address(0), "invalid address");
        require(address(gnosis_) != address(0), "invalid address");
        require(address(rsrAsset_) != address(0), "invalid address");
        require(address(implementations_.main) != address(0), "invalid address");
        require(address(implementations_.trade) != address(0), "invalid address");
        require(address(implementations_.components.assetRegistry) != address(0), "invalid address");
        require(address(implementations_.components.backingManager) != address(0), "invalid address");
        require(address(implementations_.components.basketHandler) != address(0), "invalid address");
        require(address(implementations_.components.broker) != address(0), "invalid address");
        require(address(implementations_.components.distributor) != address(0), "invalid address");
        require(address(implementations_.components.furnace) != address(0), "invalid address");
        require(address(implementations_.components.furnace) != address(0), "invalid address");
        require(address(implementations_.components.rsrTrader) != address(0), "invalid address");
        require(address(implementations_.components.rTokenTrader) != address(0), "invalid address");
        require(address(implementations_.components.rToken) != address(0), "invalid address");
        require(address(implementations_.components.stRSR) != address(0), "invalid address");

---

Deployer.sol: L109

        require(owner != address(0) && owner != address(this), "invalid owner");

Recommendation:

        require(owner != address(0), "invalid owner");
        require(owner != address(this), "invalid owner");

---

Furnace.sol: L89

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

Recommendation:

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

---

RToken.sol: L410

        require(queue.left <= endId && endId <= queue.right, "out of range");

Recommendation:

        require(queue.left <= endId, "out of range");
        require(endId <= queue.right, "out of range");

---

Similary for the following require statements incorporating &&:

RToken.sol: L590

RToken.sol: L623

RToken.sol: L741

RToken.sol: L813

RevenueTrader.sol: L72

StRSR.sol: L813

StRSR.sol: L821

Auth.sol: L181

Auth.sol: L188

Asset.sol: L50

---

---

---

RToken.sol: L344

    function whenFinished(uint256 amtRToken) private returns (uint192 finished) {

function whenFinished above is called only once, as shown below:

RToken.sol: L243

        uint192 vestingEnd = whenFinished(amtRToken); // D18{block number}

---

The following functions similarly are called only once and should be inlined:

BackingManager.sol

L154, L248

BasketHandler.sol

L75, L519

RToken.sol

L737

StRSR.sol

L543-545, L739-743

RecollateralizationLib.sol

L319-324, L428-431, L463-467

ComponentRegistry.sol

L36, L44, L52, L60, L68

L76, L84, L92, L100, L108

FiatCollateral.sol

L199-200

---

AssetRegistry.sol: L38-40

        for (uint256 i = 0; i < length; ++i) {
            _register(assets_[i]);
        }

Suggestion:

        for (uint256 i; i < length;) {
            _register(assets_[i]);

            unchecked {
              ++i;
          }
        }

---

Similarly, for the following for loops:

AssetRegistry.sol

L49-51, L127-129, L138-141

BackingManager.sol

L221, L238

BasketHandler.sol

L70, L78, L218, L262, L286

L337, L397, L416, L437, L530

L548, L553, L586, L597, L611,

L643, L653, L707, L725

Distributor.sol

L108, L133, L143

RToken.sol

L270, L303, L329, L334

L478, L501, L674, L683

L711, L757, L767, L793

RecollateralizationLib.sol

L242, L329, L437

RewardableLib.sol

L27, L67, L73

FacadeAct.sol

L76, L106, L176

L308, L317, L334

FacadeRead.sol

L101, L114, L131

L154, L273

FacadeWrite.sol

L32, L37, L64, L74

L88, L93, L110

Array.sol

L11, L12, L23

String.sol

L14

---

---

Distributor.sol: L181

    function _ensureNonZeroDistribution(uint24 rTokenDist, uint24 rsrDist) internal pure {

---

StRSRVotes.sol: L118-125

    function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {

---

Permit.sol: L10-16

    function requireSignature(
        address owner,
        bytes32 hash,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external view {

---

IDistributor.sol: L13-16

struct RevenueTotals {
    uint24 rTokenTotal; // {revShare}
    uint24 rsrTotal; // {revShare}
}

---

IDistributor.sol: L28

    event DistributionSet(address dest, uint16 rTokenDist, uint16 rsrDist);

---

IDistributor.sol: L63

    function distribution(address) external view returns (uint16 rTokenDist, uint16 rsrDist);

---

IStaticATokenLM.sol: L10-14

    struct SignatureParams {
        uint8 v;
        bytes32 r;
        bytes32 s;
    }

---

IStaticATokenLM.sol: L27-32

    function deposit(
        address recipient,
        uint256 amount,
        uint16 referralCode,
        bool fromUnderlying
    ) external returns (uint256);

---

IStaticATokenLM.sol: L102-110

    function metaDeposit(
        address depositor,
        address recipient,
        uint256 value,
        uint16 referralCode,
        bool fromUnderlying,
        uint256 deadline,
        SignatureParams calldata sigParams
    ) external returns (uint256);

---

StaticATokenLM.sol: L105-110

    function deposit(
        address recipient,
        uint256 amount,
        uint16 referralCode,
        bool fromUnderlying
    ) external override nonReentrant returns (uint256) {

---

StaticATokenLM.sol: L161-169

    function metaDeposit(
        address depositor,
        address recipient,
        uint256 value,
        uint16 referralCode,
        bool fromUnderlying,
        uint256 deadline,
        SignatureParams calldata sigParams
    ) external override nonReentrant returns (uint256) {

---

StaticATokenLM.sol: L287-293

    function _deposit(
        address depositor,
        address recipient,
        uint256 amount,
        uint16 referralCode,
        bool fromUnderlying
    ) internal returns (uint256) {

---

---