Sublime contest - Dravee's results

Lines of code

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L327

Vulnerability details

Impact

A call to transfer is done without checking the results. For certain ERC20 tokens, if insufficient tokens are present, no revert occurs but a result of "false" is returned. So it's important and also a best practice to check this.

Note that, in almost all occasions in the solution, safeTransfer is used.

Proof of Concept

Use of transfer without result-checking:

LenderPool.sol:327:        IERC20(_borrowAsset).transfer(_to, _fee);

Recommended Mitigation Steps

Always check the result of transfer or use safeTransfer

QA Report

1. Open TODOS

Consider resolving the TODOs before deploying.

PooledCreditLine/PooledCreditLine.sol:794:        // TODO: Can directly transfer to borrower and cut down on 1 transfer

2. Implicit constant visibility

Consider explicitly marking those as internal:

PooledCreditLine/PooledCreditLine.sol:33:    uint256 constant YEAR_IN_SECONDS = 365 days;
PooledCreditLine/PooledCreditLine.sol:34:    uint256 constant SCALING_FACTOR = 1e18;

3. All initialize() functions are front-runnable in the solution.

I suggest adding some access control to them:

PooledCreditLine/LenderPool.sol:
  90:     function initialize() external initializer {}

PooledCreditLine/PooledCreditLine.sol:
  407:     function initialize(

Verification/twitterVerifier.sol:
  57:     function initialize(

The one in LenderPool.sol doesn't do anything though

4. Typo in revert strings

Replace:

PooledCreditLine/LenderPool.sol:219:            revert("cant withdraw");
PooledCreditLine/PooledCreditLine.sol:484:        require(_protocolFeeCollector != address(0), 'cant be 0 address');
Verification/twitterVerifier.sol:125:        require(bytes(_userdata).length != 0, 'User doesnt exists');
Verification/twitterVerifier.sol:137:        require(bytes(_userdata).length != 0, 'User does not exists');

with

PooledCreditLine/LenderPool.sol:219:            revert("can't withdraw");
PooledCreditLine/PooledCreditLine.sol:484:        require(_protocolFeeCollector != address(0), "can't be 0 address");
Verification/twitterVerifier.sol:125:        require(bytes(_userdata).length != 0, 'User does not exist');
Verification/twitterVerifier.sol:137:        require(bytes(_userdata).length != 0, 'User does not exist');

5. A revert string shouldn't be empty

PooledCreditLine/LenderPool.sol:207:                require(_isLiquidationWithdrawn, "");

6. approve should be replace with safeApprove

approve is subject to a known front-running attack. Consider using safeApprove instead:

PooledCreditLine/LenderPool.sol:110:            SAVINGS_ACCOUNT.approve(_token, address(POOLED_CREDIT_LINE), type(uint256).max);
PooledCreditLine/PooledCreditLine.sol:777:            IERC20(_collateralAsset).approve(_strategy, _amount);
PooledCreditLine/PooledCreditLine.sol:840:        IERC20(_borrowAsset).approve(_strategy, _amount);

7. Immutable addresses should be 0-checked

Consider adding an address(0) check here:

File: LenderPool.sol
82:     constructor(
83:         address _pooledCreditLine,
84:         address _savingsAccount
85:     ) {
86:         POOLED_CREDIT_LINE = IPooledCreditLine(_pooledCreditLine); //@audit missing address(0) check on immutable address
87:         SAVINGS_ACCOUNT = ISavingsAccount(_savingsAccount); //@audit missing address(0) check on immutable address
88:     }

File: PooledCreditLine.sol
393:     constructor(address _lenderPool) {
394:         lenderPool = _lenderPool; //@audit missing address(0) check on immutable address
395:     }

8. Missing comments

The following comments are missing (see @audit tags):

PooledCreditLine/PooledCreditLine.sol:
   298:      * @param _max maximum threshold of the parameter //@audit missing @return bool
   405:      * @param _protocolFeeCollector address to which protocol fee is charged to  //@audit missing @param _borrowerVerifier & @param _verification
  1040:      * @param _id identifier for the credit line //@audit missing @return address & @return uint256

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • Findings
    • Version
      • Upgrade pragma to at least 0.8.4
      • Use Custom Errors instead of Revert Strings to save Gas after upgrading to 0.8.4+
    • Storage
      • Tighly pack storage variables
      • Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
    • Arithmetics
      • Do not use SafeMath functions on arithmetics operations that can't underflow/overflow
    • For-Loops
      • An array's length should be cached to save gas in for-loops

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Findings

Version

Upgrade pragma to at least 0.8.4

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

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

PooledCreditLine/LenderPool.sol:2:pragma solidity 0.7.6;
PooledCreditLine/PooledCreditLine.sol:2:pragma solidity 0.7.6;
Verification/twitterVerifier.sol:2:pragma solidity 0.7.6;

Use Custom Errors instead of Revert Strings to save Gas after upgrading to 0.8.4+

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

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 are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

PooledCreditLine/LenderPool.sol:73:        require(msg.sender == address(POOLED_CREDIT_LINE), '!Pooled credit line');
PooledCreditLine/LenderPool.sol:78:        require(IPooledCreditLineVerifier(creditLines[creditLineID].verifier).isUser(msg.sender), '!Verified Lender');
PooledCreditLine/LenderPool.sol:115:        require(block.timestamp < creditLines[_id].startTime, '!Collecting');
PooledCreditLine/LenderPool.sol:119:        require(_maxLent > _totalLent, 'Lending Complete');
PooledCreditLine/LenderPool.sol:207:                require(_isLiquidationWithdrawn, "");
PooledCreditLine/LenderPool.sol:284:        require(_lendingShare != 0, "!lender");
PooledCreditLine/LenderPool.sol:304:        require(_lendingShare != 0, "!lender");
PooledCreditLine/LenderPool.sol:358:        require(from != to, 'self transfer');
PooledCreditLine/LenderPool.sol:361:            require(IPooledCreditLineVerifier(creditLines[id].verifier).isUser(to) || to == address(0), '!Verified && !burn');
PooledCreditLine/LenderPool.sol:369:                require(supply >= amount, 'amount > totalSupply');
PooledCreditLine/LenderPool.sol:372:                require(creditLines[id].areTokensTransferable, '!transferrable');
PooledCreditLine/PooledCreditLine.sol:162:        require(creditLineConstants[_id].borrower == msg.sender, '!Borrower');
PooledCreditLine/PooledCreditLine.sol:170:        require(lenderPool == msg.sender, '!LenderPool');
PooledCreditLine/PooledCreditLine.sol:322:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:333:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:344:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:355:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:366:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:377:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:388:        require(_min < _max, 'Invalid limits');
PooledCreditLine/PooledCreditLine.sol:439:        require(_priceOracle != address(0), "0 address");
PooledCreditLine/PooledCreditLine.sol:454:        require(_savingsAccount != address(0), "0 address");
PooledCreditLine/PooledCreditLine.sol:469:        require(_protocolFee <= SCALING_FACTOR, "< 100%");
PooledCreditLine/PooledCreditLine.sol:484:        require(_protocolFeeCollector != address(0), 'cant be 0 address');
PooledCreditLine/PooledCreditLine.sol:499:        require(_strategyRegistry != address(0), 'CL::I zero address');
PooledCreditLine/PooledCreditLine.sol:514:        require(_borrowerVerifier != address(0), 'CL::I zero address');
PooledCreditLine/PooledCreditLine.sol:529:        require(_verification != address(0), 'CL::I zero address');
PooledCreditLine/PooledCreditLine.sol:662:        require(IVerification(verification).isUser(msg.sender, borrowerVerifier), '!verified borrower');
PooledCreditLine/PooledCreditLine.sol:663:        require(_request.borrowAsset != _request.collateralAsset, 'borrow != collateral');
PooledCreditLine/PooledCreditLine.sol:664:        require(IPriceOracle(priceOracle).doesFeedExist(_request.borrowAsset, _request.collateralAsset), '!price');
PooledCreditLine/PooledCreditLine.sol:665:        require(_request.borrowAsset != address(0) && _request.collateralAsset != address(0), "ETH not allowed");
PooledCreditLine/PooledCreditLine.sol:666:        require(IStrategyRegistry(strategyRegistry).registry(_request.lenderStrategy) != 0, '!strategy');
PooledCreditLine/PooledCreditLine.sol:667:        require(IStrategyRegistry(strategyRegistry).registry(_request.collateralStrategy) != 0, '!strategy');
PooledCreditLine/PooledCreditLine.sol:668:        require(
PooledCreditLine/PooledCreditLine.sol:672:        require(
PooledCreditLine/PooledCreditLine.sol:676:        require(
PooledCreditLine/PooledCreditLine.sol:680:        require(
PooledCreditLine/PooledCreditLine.sol:684:        require(
PooledCreditLine/PooledCreditLine.sol:688:        require(
PooledCreditLine/PooledCreditLine.sol:692:        require(
PooledCreditLine/PooledCreditLine.sol:749:        require(creditLineVariables[_id].status == CreditLineStatus.REQUESTED, '!Requested');
PooledCreditLine/PooledCreditLine.sol:767:        require(getCreditLineStatus(_id) == CreditLineStatus.ACTIVE, '!Active');
PooledCreditLine/PooledCreditLine.sol:807:        require(_amount != 0, 'amount == 0');
PooledCreditLine/PooledCreditLine.sol:808:        require(block.timestamp >= creditLineConstants[_id].startsAt, "!started");
PooledCreditLine/PooledCreditLine.sol:809:        require(_amount <= calculateBorrowableAmount(_id), '>Borrowable');
PooledCreditLine/PooledCreditLine.sol:856:        require(currentStatus == CreditLineStatus.ACTIVE || currentStatus == CreditLineStatus.EXPIRED, '!(ACTIVE || EXPIRED)');
PooledCreditLine/PooledCreditLine.sol:902:        require(msg.sender == creditLineConstants[_id].borrower, '!Borrower');
PooledCreditLine/PooledCreditLine.sol:903:        require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, '!ACTIVE');
PooledCreditLine/PooledCreditLine.sol:904:        require(creditLineVariables[_id].principal == 0, 'Principal!=0');
PooledCreditLine/PooledCreditLine.sol:961:        require(_amount <= _withdrawableCollateral, '>Withdrawable');
PooledCreditLine/PooledCreditLine.sol:1044:        require(creditLineVariables[_id].principal != 0, 'CreditLine: cannot liquidate if principal is 0');
PooledCreditLine/PooledCreditLine.sol:1045:        require(currentStatus == CreditLineStatus.ACTIVE || currentStatus == CreditLineStatus.EXPIRED, '!(ACTIVE || EXPIRED)');
Verification/twitterVerifier.sol:92:        require(bytes(userData[msg.sender].twitterId).length == 0, 'User already exists');
Verification/twitterVerifier.sol:93:        require(twitterIdMap[_twitterId] == address(0), 'Signed message already used');
Verification/twitterVerifier.sol:94:        require(block.timestamp < _timestamp + 86400, 'Signed transaction expired');
Verification/twitterVerifier.sol:106:        require(hashAddressMap[digest] == address(0), 'Hash Already Used');
Verification/twitterVerifier.sol:110:        require(signer == signerAddress, 'Invalid signature');
Verification/twitterVerifier.sol:125:        require(bytes(_userdata).length != 0, 'User doesnt exists');
Verification/twitterVerifier.sol:137:        require(bytes(_userdata).length != 0, 'User does not exists');

Storage

Tighly pack storage variables

Here, struct LendingInfo can be tightly packed from:

File: LenderPool.sol
47:     struct LendingInfo { //@audit gas: can be tightly packed by moving "bool areTokensTransferable;" after an "address" type
48:         mapping(address => LenderInfo) lenders;
49:         uint256 startTime;
50:         address token;
51:         address collateralToken;
52:         uint256 borrowLimit;
53:         address verifier;
54:         address strategy; //@audit-info 20 bytes
55:         uint256 sharesHeld; //@audit-info 32 bytes
56:         uint256 borrowerInterestShares;
57:         uint256 borrowerInterestSharesWithdrawn;
58:         uint256 yieldInterestWithdrawnShares;
59:         uint256 collateralHeld; //@audit-info 32 bytes
60:         bool areTokensTransferable; //@audit-info 1 bytes (but taking a whole 32 bytes slot by itself)
61:     }

to

File: LenderPool.sol
     struct LendingInfo {
         mapping(address => LenderInfo) lenders;
         uint256 startTime;
         address token;
         address collateralToken;
         uint256 borrowLimit;
         address verifier;
         address strategy; //@audit-info 20 bytes
         bool areTokensTransferable; //@audit-info 1 bytes (taking the same slot as "address strategy")
         uint256 sharesHeld; //@audit-info 32 bytes
         uint256 borrowerInterestShares;
         uint256 borrowerInterestSharesWithdrawn;
         uint256 yieldInterestWithdrawnShares;
         uint256 collateralHeld; //@audit-info 32 bytes
     }

Which would save 1 storage slot.

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex]; and use it.

In LenderPool.sol, there is 1 practice that should change:

• Instead of using creditLines[_id] (and creditLines[id]) directly, I suggest using a storage variable _creditLine after declaring it as such: LendingInfo storage _creditLine = creditLines[_id]; (and LendingInfo storage _creditLine = creditLines[id];, without the underscore). Instances include:

contracts/PooledCreditLine/LenderPool.sol:
  101:         creditLines[_id].startTime = block.timestamp.add(_collectionPeriod); 
  102:         creditLines[_id].token = _token; 
  103:         creditLines[_id].borrowLimit = _borrowLimit; 
  104:         creditLines[_id].verifier = _verifier; 
  105:         creditLines[_id].strategy = _strategy; 
  106:         creditLines[_id].areTokensTransferable = _areTokensTransferable; 
  115:         require(block.timestamp < creditLines[_id].startTime, '!Collecting'); 
  118:         uint256 _maxLent = creditLines[_id].borrowLimit; 
  125:         address _token = creditLines[_id].token; 
  135:         creditLines[_id].sharesHeld = SAVINGS_ACCOUNT.deposit(_token, creditLines[_id].strategy, address(this), _maxLent); 
  139:         delete creditLines[_id].startTime; 
  140:         delete creditLines[_id].borrowLimit; 
  144:         creditLines[_id].sharesHeld = creditLines[_id].sharesHeld.sub(_sharesBorrowed); 
  148:         creditLines[_id].sharesHeld = creditLines[_id].sharesHeld.add(_sharesRepaid); 
  149:         creditLines[_id].borrowerInterestShares = creditLines[_id].borrowerInterestShares.add(_interestShares); 
  159:         } else if(_creditLineStatus == uint256(CreditLineStatus.REQUESTED) && block.timestamp > creditLines[_id].startTime) {
  168:         uint256 _totalLiquidityWithdrawable = creditLines[_id].borrowLimit.sub(
  194:             block.timestamp > creditLines[_id].startTime 
  200:             address lentToken = creditLines[_id].token; 
  210:             address _strategy = creditLines[_id].strategy; 
  211:             address _lentToken = creditLines[_id].token; 
  214:             creditLines[_id].sharesHeld = creditLines[_id].sharesHeld.sub(_principalWithdrawable).sub(_interestWithdrawable); 
  232:         address _strategy = creditLines[_id].strategy; 
  233:         address _lentToken = creditLines[_id].token; 
  248:         uint256 _borrowerInterestShares = creditLines[_id].borrowerInterestShares; 
  254:                                             ).sub(creditLines[_id].lenders[_lender].borrowerInterestSharesWithdrawn); 
  257:             uint256 _notBorrowed = creditLines[_id].borrowLimit.sub(POOLED_CREDIT_LINE.getPrincipal(_id)); 
  259:             _totalInterestInShares = creditLines[_id].sharesHeld.sub(_notBorrowedInShares); 
  264:                                         .add(creditLines[_id].yieldInterestWithdrawnShares); 
  270:                                         ).sub(creditLines[_id].lenders[_lender].yieldInterestWithdrawnShares); 
  272:         creditLines[_id].lenders[_lender].yieldInterestWithdrawnShares += _yieldInterestForLender; 
  273:         creditLines[_id].lenders[_lender].borrowerInterestSharesWithdrawn += _borrowerInterestForLender; 
  274:         creditLines[_id].borrowerInterestSharesWithdrawn += _borrowerInterestForLender; 
  275:         creditLines[_id].yieldInterestWithdrawnShares += _yieldInterestForLender; 
  276:         creditLines[_id].sharesHeld -= (_yieldInterestForLender + _borrowerInterestForLender); 
  289:         uint256 _collateralLiquidated = creditLines[_id].collateralHeld; 
  295:             creditLines[_id].collateralHeld = creditLines[_id].collateralHeld.sub(_lenderCollateralShare); 
  297:             IERC20(creditLines[_id].collateralToken).safeTransfer(_lender, _lenderCollateralShare); 
  308:         creditLines[_id].collateralToken = _collateralToken; 
  309:         creditLines[_id].collateralHeld = _collateralLiquidated; 
  325:         uint256 yieldInterestOnTransferAmount = creditLines[id].lenders[from].yieldInterestWithdrawnShares.mul(amount).div(fromBalance);
  326:         uint256 borrowerInterestOnTransferAmount = creditLines[id].lenders[from].borrowerInterestSharesWithdrawn.mul(amount).div(fromBalance);
  329:             creditLines[id].lenders[from].borrowerInterestSharesWithdrawn
  330:                 =  creditLines[id].lenders[from].borrowerInterestSharesWithdrawn.sub(borrowerInterestOnTransferAmount);
  334:             creditLines[id].lenders[from].yieldInterestWithdrawnShares 
  335:                 =  creditLines[id].lenders[from].yieldInterestWithdrawnShares.sub(yieldInterestOnTransferAmount);
  340:                 creditLines[id].lenders[to].borrowerInterestSharesWithdrawn
  341:                         =  creditLines[id].lenders[to].borrowerInterestSharesWithdrawn.add(borrowerInterestOnTransferAmount);
  344:                 creditLines[id].lenders[to].yieldInterestWithdrawnShares 
  345:                         =  creditLines[id].lenders[to].yieldInterestWithdrawnShares.add(yieldInterestOnTransferAmount);
  361:             require(IPooledCreditLineVerifier(creditLines[id].verifier).isUser(to) || to == address(0), '!Verified && !burn');
  372:                 require(creditLines[id].areTokensTransferable, '!transferrable');

In PooledCreditLine.sol, there are 3 practices that should change:

• Instead of using creditLineVariables[_id] directly, I suggest using a storage variable _creditLineVariable after declaring it as such: CreditLineVariables storage _creditLineVariable = creditLineVariables[_id];. Instances include:

contracts/PooledCreditLine/PooledCreditLine.sol:
   535:         return creditLineVariables[_id].principal;
   545:         CreditLineStatus currentStatus = creditLineVariables[_id].status;
   548:             creditLineVariables[_id].status = currentStatus;
   575:         uint256 _lastPrincipalUpdateTime = creditLineVariables[_id].lastPrincipalUpdateTime;
   587:         uint256 _interestAccrued = calculateInterest(creditLineVariables[_id].principal, creditLineConstants[_id].borrowRate, _timeElapsed);
   598:         uint256 _currentDebt = (creditLineVariables[_id].principal)
   599:             .add(creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate)
   601:             .sub(creditLineVariables[_id].totalInterestRepaid);
   649:         uint256 _totalInterestAccrued = (creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate).add(_newInterestAccured);
   650:         creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate = _totalInterestAccrued;
   651:         creditLineVariables[_id].lastPrincipalUpdateTime = block.timestamp;
   652:         creditLineVariables[_id].principal = _updatedPrincipal;
   715:         creditLineVariables[_id].status = CreditLineStatus.REQUESTED;
   749:         require(creditLineVariables[_id].status == CreditLineStatus.REQUESTED, '!Requested');
   750:         creditLineVariables[_id].status = CreditLineStatus.ACTIVE;
   820:         updatePrincipal(_id, creditLineVariables[_id].principal.add(_borrowedAmount));
   859:         uint256 _totalInterestAccrued = (creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate).add(
   862:         uint256 _interestToPay = _totalInterestAccrued.sub(creditLineVariables[_id].totalInterestRepaid);
   863:         uint256 _currentPrincipal = creditLineVariables[_id].principal;
   876:             creditLineVariables[_id].principal = _currentPrincipal.sub(_principalPaid);
   877:             creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate = _totalInterestAccrued;
   878:             creditLineVariables[_id].lastPrincipalUpdateTime = block.timestamp;
   879:             creditLineVariables[_id].totalInterestRepaid = _totalInterestAccrued;
   881:             creditLineVariables[_id].totalInterestRepaid = creditLineVariables[_id].totalInterestRepaid.add(_amount);
   889:         if (creditLineVariables[_id].principal == 0) {
   891:                 creditLineVariables[_id].status = CreditLineStatus.CLOSED;
   903:         require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, '!ACTIVE');
   904:         require(creditLineVariables[_id].principal == 0, 'Principal!=0');
   905:         creditLineVariables[_id].status = CreditLineStatus.CLOSED;
  1044:         require(creditLineVariables[_id].principal != 0, 'CreditLine: cannot liquidate if principal is 0');
  1064:         creditLineVariables[_id].status = CreditLineStatus.LIQUIDATED;

• Instead of using creditLineConstants[_id] directly, I suggest using a storage variable _creditLineConstant after declaring it as such: CreditLineConstants storage _creditLineConstant = creditLineConstants[_id];. Instances include:

contracts/PooledCreditLine/PooledCreditLine.sol:
   162:         require(creditLineConstants[_id].borrower == msg.sender, '!Borrower');
   546:         if (currentStatus == CreditLineStatus.ACTIVE && creditLineConstants[_id].endsAt <= block.timestamp) {
   578:         uint256 _endTime = creditLineConstants[_id].endsAt;
   579:         uint256 _penaltyRate = creditLineConstants[_id].gracePenaltyRate;
   587:         uint256 _interestAccrued = calculateInterest(creditLineVariables[_id].principal, creditLineConstants[_id].borrowRate, _timeElapsed);
   618:             creditLineConstants[_id].collateralAsset,
   619:             creditLineConstants[_id].borrowAsset
   626:         uint256 _collateralRatio = creditLineConstants[_id].idealCollateralRatio;
   636:         uint256 _borrowLimit = creditLineConstants[_id].borrowLimit;
   716:         creditLineConstants[_id].borrower = msg.sender;
   717:         creditLineConstants[_id].borrowLimit = _request.borrowLimit;
   718:         creditLineConstants[_id].idealCollateralRatio = _request.collateralRatio;
   719:         creditLineConstants[_id].borrowRate = _request.borrowRate;
   720:         creditLineConstants[_id].borrowAsset = _request.borrowAsset;
   721:         creditLineConstants[_id].collateralAsset = _request.collateralAsset;
   723:         creditLineConstants[_id].startsAt = block.timestamp + _request.collectionPeriod;
   724:         creditLineConstants[_id].endsAt = _endsAt;
   725:         creditLineConstants[_id].defaultsAt = _endsAt + _request.defaultGracePeriod;
   726:         creditLineConstants[_id].gracePenaltyRate = _request.gracePenaltyRate;
   727:         creditLineConstants[_id].lenderStrategy = _request.lenderStrategy;
   768:         address _collateralAsset = creditLineConstants[_id].collateralAsset;
   769:         address _strategy = creditLineConstants[_id].collateralStrategy;
   808:         require(block.timestamp >= creditLineConstants[_id].startsAt, "!started");
   810:         address _borrowAsset = creditLineConstants[_id].borrowAsset;
   815:         uint256 _sharesWithdrawn = _withdrawBorrowAmount(_borrowAsset, creditLineConstants[_id].lenderStrategy, _lender, _amount);
   835:         address _strategy = creditLineConstants[_id].lenderStrategy;
   836:         address _borrowAsset = creditLineConstants[_id].borrowAsset;
   885:         uint256 _repaidInterestShares = IYield(creditLineConstants[_id].lenderStrategy).getSharesForTokens(_interestPaid, creditLineConstants[_id].borrowAsset);
   902:         require(msg.sender == creditLineConstants[_id].borrower, '!Borrower');
   919:             creditLineConstants[_id].collateralAsset,
   920:             creditLineConstants[_id].borrowAsset
   938:         address _collateralAsset = creditLineConstants[_id].collateralAsset;
   939:         address _strategy = creditLineConstants[_id].collateralStrategy;
   962:         _transferCollateral(_id, creditLineConstants[_id].collateralAsset, _amount, _toSavingsAccount);
   980:         _transferCollateral(_id, creditLineConstants[_id].collateralAsset, _withdrawableCollateral, _toSavingsAccount);
   996:             creditLineConstants[_id].collateralAsset,
   997:             creditLineConstants[_id].borrowAsset
  1004:             .mul(creditLineConstants[_id].idealCollateralRatio)
  1021:         address _strategy = creditLineConstants[_id].collateralStrategy;
  1046:         address _collateralAsset = creditLineConstants[_id].collateralAsset;
  1051:         if (currentCollateralRatio < creditLineConstants[_id].idealCollateralRatio) {
  1053:         } else if (block.timestamp >= creditLineConstants[_id].defaultsAt) {
  1055:             address _borrowAsset = creditLineConstants[_id].borrowAsset;
  1080:         address _collateralAsset = creditLineConstants[_id].collateralAsset;
  1081:         address _borrowAsset = creditLineConstants[_id].borrowAsset;

• Instead of using collateralShareInStrategy[_id] directly, I suggest using a storage variable _collateralShareInStrategy after declaring it as such: mapping(uint => uint256) storage _collateralShareInStrategy = collateralShareInStrategy[_id];. Instances include:

contracts/PooledCreditLine/PooledCreditLine.sol:
   781:         collateralShareInStrategy[_id][_strategy] = collateralShareInStrategy[_id][_strategy].add(_sharesDeposited);
   941:         uint256 _collateralShares = collateralShareInStrategy[_id][_strategy];
  1025:         collateralShareInStrategy[_id][_strategy] = collateralShareInStrategy[_id][_strategy].sub(

Arithmetics

Do not use SafeMath functions on arithmetics operations that can't underflow/overflow

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 not using the SafeMath library. This would avoid making an unnecessary check.

Instances include (see @audit tags):

contracts/PooledCreditLine/LenderPool.sol:
  122          if (_totalLent.add(_amount) > _maxLent) {
  123:             _amountToLend = _maxLent.sub(_totalLent); //@audit gas: safemath not needed here due to L122

contracts/PooledCreditLine/PooledCreditLine.sol:
   581          if (_lastPrincipalUpdateTime <= _endTime && block.timestamp > _endTime) {
   582:             penalityInterest = (block.timestamp.sub(_endTime)).mul(_penaltyRate).div(SCALING_FACTOR); //@audit gas: safemath not needed here due to L581

   822          uint256 _protocolFee = _borrowedAmount.mul(protocolFeeFraction).div(SCALING_FACTOR);
   823:         _borrowedAmount = _borrowedAmount.sub(_protocolFee); //@audit gas: safemath not needed here due to L822 (_protocolFee <= _borrowedAmount due to bounded protocolFeeFraction)

   874          if (_amount > _interestToPay) {
   875:             _principalPaid = _amount.sub(_interestToPay); //@audit gas: safemath not needed here due to L874

  1009          if (_collateralNeeded >= _totalCollateralTokens) {
  1010              return 0;
  1011          }
  1012:         return _totalCollateralTokens.sub(_collateralNeeded); //@audit gas: safemath not needed here due to L1009-L1010

For-Loops

An array's length should be cached to save gas in for-loops

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

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

PooledCreditLine/LenderPool.sol:359:        for (uint256 i; i < ids.length; ++i) {