code-423n4 / 2022-03-sublime-findings

0 stars 0 forks source link

QA Report #23

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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

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:

terminatdterminated

pooleedpooled

reqeuestedrequested

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.

ritik99 commented 2 years ago

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 commented 2 years ago

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

HardlyDifficult commented 2 years ago

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