Sublime contest - robee's results

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    LenderPool.sol, initialize, 220
    PooledCreditLine.sol, initialize, 608
    twitterVerifier.sol, initialize, 83

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    LenderPool.sol.initialize _owner
    PooledCreditLine.sol.initialize _owner

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    PooledCreditLine.updateProtocolFeeCollector (_protocolFeeCollector)
    LenderPool.initialize (_startFeeFraction)
    PooledCreditLine.updateProtocolFeeFraction (_protocolFeeFraction)
    PooledCreditLine.initialize (_protocolFeeCollector)
    LenderPool.updateStartFeeFraction (_startFeeFraction)
    LenderPool._updateStartFeeFraction (_startFeeFraction)
    PooledCreditLine._updateProtocolFeeFraction (_protocolFeeFraction)
    PooledCreditLine.initialize (_protocolFeeFraction)
    PooledCreditLine._updateProtocolFeeCollector (_protocolFeeCollector)

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    LenderPool.sol._calculateInterestToWithdraw _strategy
    twitterVerifier.sol._updateSignerAddress _signerAddress
    twitterVerifier.sol.updateVerification _verification
    LenderPool.sol._calculatePrincipalWithdrawable _lender

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in PooledCreditLine.sol line 1024: IERC20(_borrowAsset).approve(_strategy, _amount);

Deprecated safeApprove in LenderPool.sol line 334: IERC20(_borrowAsset).safeApprove(_strategy, _amount);

Deprecated safeApprove in PooledCreditLine.sol line 754: IERC20(_collateralAsset).approve(_strategy, _amount);

Deprecated safeApprove in LenderPool.sol line 266: SAVINGS_ACCOUNT.approve(_borrowAsset, address(POOLED_CREDIT_LINE), type(uint256).max);

Title: Loss Of Precision Severity: Low Risk

This issue is about arithmetic computation that could have been done more percise. The following are places in the codebase in which you multiplied after the divisions. Doing the multiplications at start lead to more accurate calculations. This is a list of places in the code that this appears (Solidity file, line number, actual line):

    PooledCreditLine.sol, 943, _maxPossible = _totalCollateralToken.mul(_ratioOfPrices).div(_collateralRatio).mul(SCALING_FACTOR).div(10**_decimals);
    PooledCreditLine.sol, 822, uint256 _collateralNeeded = _currentDebt .mul(pooledCreditLineConstants[_id].idealCollateralRatio) .div(_ratioOfPrices) .mul(10**_decimals) .div(SCALING_FACTOR); 
    PooledCreditLine.sol, 1245, _currentCollateralRatio = calculateTotalCollateralTokens(_id).mul(_ratioOfPrices).div(_currentDebt).mul(SCALING_FACTOR).div( 10**_decimals );

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    PooledCreditLine.sol, getStatus is missing a reentrancy modifier
    PooledCreditLine.sol, updateCollectionPeriodLimits is missing a reentrancy modifier
    PooledCreditLine.sol, updateBorrowLimitLimits is missing a reentrancy modifier
    LenderPool.sol, requestCancelled is missing a reentrancy modifier

Title: Two arrays length mismatch Severity: Low/Med Risk

The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior. Consider making this a medium risk please.

    LenderPool.sol, _beforeTokenTransfer  ['ids', 'amounts'] 

Title: Must approve 0 first Severity: Low Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first LenderPool.sol, 334, IERC20(_borrowAsset).safeApprove(_strategy, _amount);

approve without approving 0 first LenderPool.sol, 266, SAVINGS_ACCOUNT.approve(_borrowAsset, address(POOLED_CREDIT_LINE), type(uint256).max);

approve without approving 0 first PooledCreditLine.sol, 1024, IERC20(_borrowAsset).approve(_strategy, _amount);

approve without approving 0 first PooledCreditLine.sol, 754, IERC20(_collateralAsset).approve(_strategy, _amount);

Title: approve return value is ignored Severity: Low Risk

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens. The list of occurrences in format (solidity file, line number, actual line) PooledCreditLine.sol, 1024, IERC20(_borrowAsset).approve(_strategy, _amount);

LenderPool.sol, 266, SAVINGS_ACCOUNT.approve(_borrowAsset, address(POOLED_CREDIT_LINE), type(uint256).max);

PooledCreditLine.sol, 754, IERC20(_collateralAsset).approve(_strategy, _amount);

Title: Unnecessary cast Severity: Gas

    Request PooledCreditLine.sol.request - unnecessary casting Request(_request)

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    PooledCreditLine.sol, getPrincipal

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    twitterVerifier.sol (L#126) - require(block.timestamp < _timestamp + signValidity, 'RS3'); 
    PooledCreditLine.sol (L#683) - uint256 _endsAt = block.timestamp + _request.collectionPeriod + _request.duration;
    PooledCreditLine.sol (L#684) - _clc.startsAt = block.timestamp + _request.collectionPeriod;

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    just change to unchecked: LenderPool.sol, i, 670

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    PooledCreditLine.sol, _calculateInterest, { return (_principal.mul(_borrowRate).mul(_timeElapsed).div(YEAR_IN_SECONDS).div(SCALING_FACTOR)); }

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    LenderPool.sol, ids, 670

Title: Upgrade pragma to at least 0.8.4 Severity: GAS

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

    LenderPool.sol
    PooledCreditLine.sol
    twitterVerifier.sol

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    LenderPool.sol, _beforeTokenTransfer

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    PooledCreditLine.sol, _notifyRequest
    PooledCreditLine.sol, _borrow
    PooledCreditLine.sol, _withdrawBorrowAmount
    LenderPool.sol, _rebalanceInterestWithdrawn
    PooledCreditLine.sol, updateStateOnPrincipalChange
    PooledCreditLine.sol, _limitBorrowedInUSD
    PooledCreditLine.sol, _repay
    PooledCreditLine.sol, _createRequest

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    twitterVerifier.initialize (_version)
    twitterVerifier.registerSelf (_tweetId)
    twitterVerifier.initialize (_name)

Title: Cache powers of 10 used several times Severity: GAS

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

PooledCreditLine.sol, 389 : You should cache the used power of 10 as constant state variable since it's used several times (6): uint256 _poolsizeInUSD = _borrowLimit.mul(_ratioOfPrices).div(10**_decimals);

PooledCreditLine.sol, 1246 : You should cache the used power of 10 as constant state variable since it's used several times (6): 10**_decimals

PooledCreditLine.sol, 825 : You should cache the used power of 10 as constant state variable since it's used several times (6): .mul(10**_decimals)

PooledCreditLine.sol, 394 : You should cache the used power of 10 as constant state variable since it's used several times (6): uint256 _minBorrowLimitInUSD = _minBorrowAmount.mul(_ratioOfPrices).div(10**_decimals);

PooledCreditLine.sol, 1259 : You should cache the used power of 10 as constant state variable since it's used several times (6): uint256 _collateralTokens = (_borrowTokens.mul(_ratioOfPrices).div(10**_decimals));

PooledCreditLine.sol, 943 : You should cache the used power of 10 as constant state variable since it's used several times (6): _maxPossible = _totalCollateralToken.mul(_ratioOfPrices).div(_collateralRatio).mul(SCALING_FACTOR).div(10**_decimals);

Title: Unnecessary Reentrancy Guards Severity: GAS

Where there is onlyOwner or Initializer modifer, the reentrancy gaurd isn't necessary (unless you don't trust the owner or the deployer, which will lead to full security breakdown of the project and we believe this is not the case) This is a list we found of such occurrences:

    PooledCreditLine.sol no need both nonReentrant and onlyOwner modifiers in terminate

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    LenderPool.sol, _beforeTokenTransfer
    LenderPool.sol, _withdrawInterest
    PooledCreditLine.sol, _equivalentCollateral
    LenderPool.sol, _withdrawLiquidity
    LenderPool.sol, _updateStartFeeFraction
    PooledCreditLine.sol, _withdrawBorrowAmount
    LenderPool.sol, _calculatePrincipalWithdrawable
    LenderPool.sol, _withdrawLiquidation
    PooledCreditLine.sol, _updateProtocolFeeFraction
    PooledCreditLine.sol, isWithinLimits
    PooledCreditLine.sol, _updatePriceOracle
    PooledCreditLine.sol, _withdrawCollateral
    PooledCreditLine.sol, _transferCollateral
    LenderPool.sol, _calculateInterestToWithdraw
    PooledCreditLine.sol, _updateProtocolFeeCollector
    PooledCreditLine.sol, _updateVerification
    LenderPool.sol, _accept
    PooledCreditLine.sol, _notifyRequest
    PooledCreditLine.sol, _updateSavingsAccount
    PooledCreditLine.sol, _borrow
    LenderPool.sol, _rebalanceInterestWithdrawn
    PooledCreditLine.sol, updateStateOnPrincipalChange
    PooledCreditLine.sol, _calculateInterest
    PooledCreditLine.sol, _limitBorrowedInUSD
    PooledCreditLine.sol, _repay
    PooledCreditLine.sol, _updateStrategyRegistry
    PooledCreditLine.sol, _createRequest