QA Report

[code423n4]

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: code-423n4/2021-09-sushimiso-findings#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);

[ritik99]

Some of the issues mentioned by the warden are relevant/acknowledged (issue no. 1, 2, 4, 5, 7, 10).

Needs further input/disputed:

1. "Does not validate the input fee parameter": It is not clear what kind of values should we check for. Would be helpful if the warden could elaborate
2. "Loss Of Precision": The order has been set to prevent overflows. Imprecision errors are insignificant considering most tokens have large decimals places
3. "Two arrays length mismatch": Would like some more details from the warden for this issue
4. "Must approve 0 first": In most instances, the entire allowance is utilized in the subsequent transfer, which makes the allowance 0. Thus setting it to 0 is not really necessary, although we'll recheck all instances

[ritik99]

Update: for "Two arrays length mismatch", the check ids.length == amounts.length is performed internally within ERC1155 before the call to _ beforeTokenTransfer