Platform: Code4rena
Start Date: 29/03/2022
Pot Size: $30,000 USDC
Total HM: 6
Participants: 24
Period: 3 days
Judge: HardlyDifficult
Total Solo HM: 4
Id: 101
League: ETH
Rank: 3/24
Findings: 2
Award: $2,876.49
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: kyliek
https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L312 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L336
An attacker could keep track of the totalSupply of each LenderPool to see if it is more than the minBorrowAmount. If so, at startTime, which is pre-announced, the attacker could call start, which will trigger SAVINGS_ACCOUNT.deposit() of the entire pool assets to mint LP tokens from external strategy, for example, in CompoundYield.
There is potentially a big sum depositing into a known Compound cToken contract at a known fixed time. Thus, the attacker could prepare the pool by depositing a fair sum first to lower the exchange rate before calling start in lenderPool. Hence, the deposit of the entire pool could be at a less favourable rate. This also applies to other potential strategies that are yet to be integrated. For example, in Curve pool, the attacker could prime the pool to be very imbalanced first and trigger the deposit and then harvest the arbitrage bonus by bringing the pool back to balance.
This attack can happen once only when the pooledCreditLine becomes active for each new lenderPool.
Step 1: When a new LenderPool started, note the borrowAsset token and its strategy target pool, as well as the collection period (i.e. start time)
Step 2: Closer to the start time block number, if totalSupply of the lenderPool is bigger than the minBorrowAmount, prepare a good sum to manipulate the target strategy pool for unfavorable exchange rate or arbitrage opportunity afterwords.
Step 3: Call start function before others, also put in his own address to _to to pocket the protocol fee.
Step 4: Depending on the strategy pool, harvest arbitrage. Or perhaps just withdraw one's money from Step 2 for griefing.
Add access control on start, e.g. only borrower can call through pooledCreditLine.
#0 - ritik99
2022-04-12T13:06:45Z
We're not sure how this leads to an attack. Fluctuation in yield is known and expected for most yield strategies like Compound. This, however, does not cause a loss of funds. An attacker instantly withdrawing their liquidity doesn't affect others because interest rates are not "locked in" on the yield strategies. There are no exchanges taking place. Some more info on possible attacks might help
#1 - HardlyDifficult
2022-05-02T14:48:18Z
Followed up with the warden and there appears to be a way to leak value by debalancing the pool before start and then rebalancing to extract some profit. This could be done with a flashbot for example to limit exposure.
The warden referenced https://github.com/yearn/yearn-security/blob/master/disclosures/2021-02-04.md as an example of something similar.
#2 - ritik99
2022-05-18T06:56:56Z
Had a discussion with the warden and the judge regarding this issue. For Compound in particular we checked that the exchange rate does not change upon deposits or withdrawals. Thus, sandwiching a call to start() couldn't possibly lead to an attack vector. Additionally, because of another issue related to start fees #19, we decided to restrict start() to be callable only by the borrower.
However, the yield strategies that we whitelist might still be internally susceptible to this attack (for eg, https://github.com/yearn/yearn-security/blob/master/disclosures/2021-02-04.md). We'll be incorporating checks for this while onboarding strategies. Picking riskier strategies is optional and not enforced at the contract-level
285.8952 USDC - $285.90
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.
withdrawInterest in LenderPool to reduce compounding effect for lenders in strategywithdraw 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.
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.
collateralAsset is not initialised in createWhen 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
borrowLimit changed at accept stage, thus not a constantThe borrower proposed a borrowLimit recorded in the pooledCreditLineConstants[id] in the creation of the request.
Then it is recorded as a constant too in the corresponding lenderPool in create().
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.
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.
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.
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
_clc.collateralAssetStrategy = _request.collateralAssetStrategy;
This line is duplicated twice in the same block.
borrowToken is used sometimes to refer to borrowAsset. Replace borrowTokenwith borrowAsset to be consistent throughout.
Instances are
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
#0 - ritik99
2022-04-12T19:43:16Z
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
#1 - HardlyDifficult
2022-04-26T15:19:28Z