QA Report

[code423n4]

General comments

The codebase is generally very well commented and functions grouped together in a very sensible and easy to read fashion. Despite the complexity of the project, the scope for this audit is limited to Pooled Credit Lines. I am generally impressed by the code quality and the genuine effort the developers have put in to make it as easy to comprehend as possible.

LOW RISK

[L01] Anyone can call withdrawInterest in LenderPool to reduce compounding effect for lenders in strategy

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

withdraw function has no access control. Since lender address is on chain, anyone could call this function to withdraw a lender's interest to a lender's account.

This will not benefit the caller but will reduce the compounding effect for lenders in their strategy. It is better to use msg.sender so that only caller who is a lender can withdraw its own interest.

NON-CRITICAL informational

[N01] Risk accumulation by opening up multiple pooledCreditLine back to back

There is no restriction on the number of concurrent pooledCreditLine a borrower can request. Thus, this could create on its own a dynamic where the borrower opens a line to feed a previous line to get good credits with lenders for under-collateralised loans on the same platform and at the same time accumulate risks for default at some point in the future. That would be the point that the lenders suffer the most.

[N02] collateralAsset is not initialised in create

When a lenderPool is created, the pooledCLConstants are initialised. However, collateralAsset is not initialised at the start but only updated later in liquidate. I wonder what is the advantage of this since the borrower will supply this value at the time of request creation.

Affected code lines: https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L681

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

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

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

[N03] borrowLimit changed at accept stage, thus not a constant

The borrower proposed a borrowLimit recorded in the pooledCreditLineConstants[id] in the creation of the request.

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

Then it is recorded as a constant too in the corresponding lenderPool in create().

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

Later on, depending on the amount collected from the lenders, this value is updated to be all available borrowAssets contributed to the lenderPool minus fees.

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

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

Thus, it is rather confusing that this is included as a pool constant instead of a pool variable as it has changed value in the process.

[N04] Cryptic revert message throughout

The revert messages are in code and it is very cryptic to users when a transaction didn't go through. For example, https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L209 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L217

I see error codes are explained here https://docs.sublime.finance/sublime-docs/smart-contracts/pooled-credit-lines#error-codes in the doc. However, it would be nice to have a simple explainable on-chain message from UX viewpoint.

[N05] Struct and mapping names too similar

The mapping and the corresponding Struct names are too similar, except for the first letter in capital or not. These get a bit confusing and could lead to errors too.

Instances are https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L193 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L198

[N06] Duplicated code

 _clc.collateralAssetStrategy = _request.collateralAssetStrategy;

This line is duplicated twice in the same block.

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

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

[N07] Naming not consistent

borrowToken is used sometimes to refer to borrowAsset. Replace borrowTokenwith borrowAsset to be consistent throughout.

Instances are

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

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

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

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

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

[N08] Empty returns are not informative

There are a few instances where a function gives an empty return without any extra information. It would be useful to give more context on which control if condition is reached.

Instances are https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L324 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L687

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

[ritik99]

Happy to hear that the comments helped and that the contracts are readable. Thank you for the comment!

The suggestions made by the warden are all relevant. [L01] is a known trade-off, we'd implemented it this way so that it is possible to abstract the chore of calling withdrawals periodically by a third-party service provider/keeper. However, we might consider restricting the caller to lenders (who can only withdraw their own interests)

The non-critical suggestions are all valid, we would try to incorporate them into our code. The report is of high quality

[HardlyDifficult]

Merging with https://github.com/code-423n4/2022-03-sublime-findings/issues/65