QA Report

[code423n4]

Codebase Impressions & Summary

Overall, code quality for the PooledCreditLine contracts is great. Majority of the logic lies in the 2 contracts PooledCreditLine and LenderPool, with a small part on the twitterVerifier that handles verification via Twitter.

Complexity

The project is a little high in complexity because of there are quite a number of possible states that a pooled credit line can have over its lifecycle, which means state handling has to be thoroughly scrutinised between transitions. The handling of interest rate calculations, borrower and lender shares accounting, and shares <> amounts conversions for integration with the saving account and strategies are other factors that raise the complexity. A lot of logic and functionality is thus packed into the 2 contracts that makes this 3 day contest feel underscoped.

Documentation

The documentation provided was adequate in understanding the pool credit line functionality. Documentation about the termination functionality and start and protocol fees were unfortunately omitted. It would be great to add them in.

It would also have been great if inline comments were added to the _calculateInterestToWithdraw() and _rebalanceInterestWithdrawn() functions as I spent quite a bit of time deciphering what these functions were doing. Nevertheless, there were sufficient inline comments for the other parts of the contracts.

Tests

Tests were unfortunately omitted because last minute changes were made to the contract and “the tests couldn't be modified to meet those changes in time for the contest. In order to not confuse people we decided it was best to remove the tests from the final release”. It would have been a nice to have so that coverage can be run, and for us to quickly spin up POCs. I’m not sure how feasible it would have been to postpone the contest by a few days so that tests could be modified, but it would’ve been beneficial.

Responsiveness

I would like to commend Ritik for his quick responses to my DMs and question on the Discord channel! =)

Low Severity Findings

L01: Discrepancy between recorded borrow amount in event and state update

Line References

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

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

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

Description

A protocol fee is taken whenever the borrower decides to borrow more tokens. The state update includes this protocol fee, but the amount emitted in the BorrowedFromPooledCreditLine event does not.

In my opinion, since the protocol fee should be included in the emitted event.

Recommended Mitigation Steps

emit BorrowedFromPooledCreditLine(_id, _borrowedAmount.add(protocolFee));

L02: Use upgradeable version of OZ contracts

Line References

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

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

Description

It is recommended to use the upgradeable version of OpenZeppelin contracts, as some contracts like ReentrancyGuard has a constructor method to set the initial status as _NOT_ENTERED = 1. The status would currently defaults to 0, which fortunately doesn’t break the nonReentrant() functionality.

Nevertheless, it would be recommended to use the upgradeable counterparts instead.

L03: calculatePrincipalWithdrawable() should return user balance for CANCELLED status

Line References

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

Description

In the event the borrower cancels his borrow request, the principal withdrawable by the lender should be the liquidity he provided, but the function returns 0 instead.

Recommended Mitigation Steps

Add the CANCELLED case in the second if branch.

else if (
  (
    _status == PooledCreditLineStatus.REQUESTED &&
    block.timestamp > pooledCLConstants[_id].startTime &&
    totalSupply[_id] < pooledCLConstants[_id].minBorrowAmount
  ) || (_status == PooledCreditLineStatus.CANCELLED)
) {
  return balanceOf(_lender, _id);
}

L04: Use continue instead of return in _beforeTokenTransfer()

Line References

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

Description

Should the contract be upgraded to use _mintBatch() in the future, the function will terminate prematurely after minting the first id.

Recommended Mitigation Steps

Replace return with continue.

if (from == address(0)) {
  continue;
}

L05: Token approval issues

Description

• safeApprove() has been deprecated in favour of safeIncreaseAllowance() and safeDecreaseAllowance()
• using approve() might fail because some tokens (eg. USDT) don’t work when changing the allowance from an existing non-zero allowance value

Recommended Mitigation Steps

Update instances of approve() and safeApprove() to safeIncreaseAllowance().

Non-Critical Findings

NC01: Typos

Description

Do a CTRL / CMD + F for the following errors:

terminatd → terminated

pooleed → pooled

reqeuested → requested

NC02: Definition mix-up in documentation

Reference

https://docs.sublime.finance/sublime-docs/the-protocol/pooled-credit-lines#creating-a-pooled-credit-line

Description

The definitions for the Collateral Savings Strategy and Borrowed Asset Savings Strategy have been mixed up.

Recommended Mitigation Steps

9. Collateral Savings Strategy: Savings strategy where any collateral locked in by the borrower will be deployed, e.g., all WBTC deposited by the borrower as collateral could be locked in Aave to earn interest
10. Borrowed Asset Savings Strategy: Savings strategy where any idle liquidity in the credit line will be deployed, e.g., all the idle USDC in the pooled credit line can be deployed to Compound

NC03: Inconsistent Naming

Description

It would be great to have variable naming kept consistent for better readibility.

• _lendingShare, _liquidityProvided to represent balanceOf(msg.sender, _id);
• withdrawnShares vs sharesWithdrawn

[ritik99]

Thanks for the comments! We'll definitely be updating our documentation to make it more detailed, both the external docs as well as inline comments.

All the issues mentioned by the warden are relevant. Usually, where approve() is used the allowance is used entirely in the subsequent transfer step, so it shouldn't be an issue, although we'll recheck all such instances. The report is of high quality.

[HardlyDifficult]

Merged with #20

[HardlyDifficult]

This report clear / easy to read. The intro is a great addition to provide some high level feedback.

• L01: Discrepancy between recorded borrow amount in event and state update
  Non-critical: This is somewhat arbitrary, but useful feedback to consider. Depending on the use case for consumers of this event, it may be useful to emit both _borrowedAmount and protocolFee as separate params as well.
• L02: Use upgradeable version of OZ contracts
  Non-critical: This is a best practice but as the warden points out it will not break anything in the current state. Switching to ReentrancyGuardUpgradeable would save gas on first usage.
• L03: calculatePrincipalWithdrawable() should return user balance for CANCELLED status
  Low-risk: This impacts an external getter that in the original form may return misleading results after a request is canceled.
• L04: Use continue instead of return in _beforeTokenTransfer()
  Low-risk: If the return in this loop is executed than other tokenIds being transferred would skip the require checks and possibly some expected state updates. However given that the code currently does not batch mint this effectively has no impact but could crop up unexpectedly after an upgrade as the warden pointed out.
• L05: Token approval issues
  Non-issue: Several wardens pointed to this concern. The way the contract is implemented, approval always resets back to 0 after the transfer so the failure scenario would not arise. It's a good consideration though and something to be careful about to ensure that assumption holds true.
• NC01: Typos
  Non-critical: Always nice to fix up the spelling errors.
• NC02: Definition mix-up in documentation
  Non-critical: This is a nice catch to improve the documentation. Ramping up on a new protocol takes time and changes like this can help the reader create the right mental models.
• NC03: Inconsistent Naming
  Non-critical: Naming is always hard to do well. Improving internal consistency does help the reader.