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: 1/24
Findings: 5
Award: $14,361.92
🌟 Selected for report: 4
🚀 Solo Findings: 2
🌟 Selected for report: hickuphh3
8635.2861 USDC - $8,635.29
https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L594-L599 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L399-L404
The _principalWithdrawable calculated will be more than expected if _start() is invoked with a non-zero start fee, because the borrow limit is reduced by the fee, resulting in totalSupply[id] not being 1:1 with the borrow limit.
function _calculatePrincipalWithdrawable(uint256 _id, address _lender) internal view returns (uint256) { uint256 _borrowedTokens = pooledCLConstants[_id].borrowLimit; uint256 _totalLiquidityWithdrawable = _borrowedTokens.sub(POOLED_CREDIT_LINE.getPrincipal(_id)); uint256 _principalWithdrawable = _totalLiquidityWithdrawable.mul(balanceOf(_lender, _id)).div(_borrowedTokens); return _principalWithdrawable; }
Assume the following conditions:
100_000 tokens: totalSupply[_id] = 100_000borrowLimit = 99_000 because of a 1% startFeeWhen Alice attempts to withdraw her tokens, the _principalWithdrawable amount is calculated as
_borrowedTokens = 99_000 _totalLiquidityWithdrawable = 99_000 - 0 = 99_000 _principalWithdrawable = 99_000 * 100_000 / 99_000 = 100_000
This is more than the available principal amount of 99_000, so the withdrawal will fail.
One hack-ish way is to save the initial supply in minBorrowAmount (perhaps rename the variable to minInitialSupply) when the credit line is accepted, and replace totalSupply[_id] with it.
The other places where minBorrowAmount are used will not be affected by the change because:
startTime has been zeroed, so start() cannot be invoked (revert with error S1)
credit line status would have been changed to ACTIVE and cannot be changed back to REQUESTED, meaning the check below will be false regardless of the value of minBorrowAmount.
_status == PooledCreditLineStatus.REQUESTED && block.timestamp > pooledCLConstants[_id].startTime && totalSupply[_id] < pooledCLConstants[_id].minBorrowAmount
Code amendment example:
function _accept(uint256 _id, uint256 _amount) internal { ... // replace delete pooledCLConstants[_id].minBorrowAmount; with the following: pooledCLConstants[_id].minInitialSupply = totalSupply[_id]; } // update comment in _withdrawLiquidity // Case 1: Pooled credit line never started because desired amount wasn't reached // state will never revert back to REQUESTED if credit line is accepted so this case is never run function _calculatePrincipalWithdrawable(uint256 _id, address _lender) internal view returns (uint256) { uint256 _borrowedTokens = pooledCLConstants[_id].borrowLimit; uint256 _totalLiquidityWithdrawable = _borrowedTokens.sub(POOLED_CREDIT_LINE.getPrincipal(_id)); // totalSupply[id] replaced with minInitialSupply uint256 _principalWithdrawable = _totalLiquidityWithdrawable.mul(balanceOf(_lender, _id)).div(minInitialSupply); return _principalWithdrawable; }
In terminate(), the shares withdrawable can simply be _sharesHeld.
function terminate(uint256 _id, address _to) external override onlyPooledCreditLine nonReentrant { address _strategy = pooledCLConstants[_id].borrowAssetStrategy; address _borrowAsset = pooledCLConstants[_id].borrowAsset; uint256 _sharesHeld = pooledCLVariables[_id].sharesHeld; SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _sharesHeld, false); delete pooledCLConstants[_id]; delete pooledCLVariables[_id]; }
2331.5273 USDC - $2,331.53
_principalWithdrawable is denominated in the borrowAsset, but subsequently treats it as the share amount to be withdrawn.
// _notBorrowed = borrowAsset amount that isn't borrowed // totalSupply[_id] = ERC1155 total supply of _id // _borrowedTokens = borrower's specified borrowLimit uint256 _principalWithdrawable = _notBorrowed.mul(totalSupply[_id]).div(_borrowedTokens); SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _principalWithdrawable.add(_totalInterestInShares), false);
The amount of shares to withdraw can simply be _sharesHeld.
Note that this comes with the assumption that terminate() is only called when the credit line is ACTIVE or EXPIRED (consider ensuring this condition on-chain), because _sharesHeld excludes principal withdrawals, so the function will fail once a lender withdraws his principal.
function terminate(uint256 _id, address _to) external override onlyPooledCreditLine nonReentrant { address _strategy = pooledCLConstants[_id].borrowAssetStrategy; address _borrowAsset = pooledCLConstants[_id].borrowAsset; uint256 _sharesHeld = pooledCLVariables[_id].sharesHeld; SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _sharesHeld, false); delete pooledCLConstants[_id]; delete pooledCLVariables[_id]; }
#0 - liveactionllama
2022-03-31T15:57:24Z
I've updated the title of this submission, per request from warden hickuphh3:
I accidentally put the code line reference https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L404-L406 as the title instead. Kindly help change it to "PooledCreditLine: termination likely fails because _principleWithdrawable is treated as shares"
🌟 Selected for report: hickuphh3
2590.5858 USDC - $2,590.59
Interest is calculated as
(_principal.mul(_borrowRate).mul(_timeElapsed).div(YEAR_IN_SECONDS).div(SCALING_FACTOR));
It is possible for the calculated interest to be zero for principal tokens with small decimals, such as EURS (2 decimals). Accumulated interest can therefore be zero by borrowing / repaying tiny amounts frequently.
Assuming a borrow interest rate of 5% (5e17) and principal borrow amount of 10_000 EURS (10_000 * 1e2 = 1_000_000), the interest rate calculated would be 0 if principal updates are made every minute (around 63s).
// in this example, maximum duration for interest to be 0 is 63s 1_000_000 * 5e17 * 63 / (86400 * 365) / 1e18 = 0.99885 // = 0
While plausible, this method of interest evasion isn’t as economical for tokens of larger decimals like USDC and USDT (6 decimals).
Take caution when allowing an asset to be borrowed. Alternatively, scale the principal amount to precision (1e18) amounts.
#0 - ritik99
2022-04-06T11:30:30Z
Tokens are whitelisted to ensure precision issues would not occur. Hence the issue is improbable and doesn't occur for widely used tokens as the decimals are generally higher.
Since there is no direct attack path (the steps required for this to occur would be: the token would first have to be whitelisted -> a loan request created using it -> lenders supply sufficient liquidity for this request to go active) and is, in essence, a value leak, we would suggest reducing the severity of the issue to (1) Low / (2) Medium
700.7235 USDC - $700.72
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.
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.
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 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.
I would like to commend Ritik for his quick responses to my DMs and question on the Discord channel! =)
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.
emit BorrowedFromPooledCreditLine(_id, _borrowedAmount.add(protocolFee));
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.
calculatePrincipalWithdrawable() should return user balance for CANCELLED statusIn 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.
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); }
continue instead of return in _beforeTokenTransfer()Should the contract be upgraded to use _mintBatch() in the future, the function will terminate prematurely after minting the first id.
Replace return with continue.
if (from == address(0)) { continue; }
safeApprove() has been deprecated in favour of safeIncreaseAllowance() and safeDecreaseAllowance()approve() might fail because some tokens (eg. USDT) don’t work when changing the allowance from an existing non-zero allowance valueUpdate instances of approve() and safeApprove() to safeIncreaseAllowance().
Do a CTRL / CMD + F for the following errors:
terminatd → terminated
pooleed → pooled
reqeuested → requested
The definitions for the Collateral Savings Strategy and Borrowed Asset Savings Strategy have been mixed up.
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
It would be great to have variable naming kept consistent for better readibility.
_lendingShare, _liquidityProvided to represent balanceOf(msg.sender, _id);withdrawnShares vs sharesWithdrawn#0 - ritik99
2022-04-13T08:13:29Z
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.
#1 - HardlyDifficult
2022-04-19T00:34:56Z
#2 - HardlyDifficult
2022-05-13T13:51:53Z
This report clear / easy to read. The intro is a great addition to provide some high level feedback.
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.103.7854 USDC - $103.79
The case if (_maxPossible <= _currentDebt) return 0; can be shifted up a couple of lines to avoid the additional storage reads of the borrow limit and principal.
uint256 _maxPossible = type(uint256).max; if (_collateralRatio != 0) { _maxPossible = _totalCollateralToken.mul(_ratioOfPrices).div(_collateralRatio).mul(SCALING_FACTOR).div(10**_decimals); } // shifted up if (_maxPossible <= _currentDebt) return 0; uint256 _borrowLimit = pooledCreditLineConstants[_id].borrowLimit; uint256 _principal = pooledCreditLineVariables[_id].principal;
CRLC2 and S2 are essentially the same checks because pooledCreditLineConstants[_id].startsAt and pooledCLConstants[_id].startTime have the same value.
Either one of the checks can be removed.
from != address(0) check is redundant in _rebalanceInterestWithdrawn()_rebalanceInterestWithdrawn() is called just once in a couple of lines above in _beforeTokenTransfer(). Note that the function won’t be invoked if from == address(0).
if (from == address(0)) { // note: replaced with continue; continue; }
Hence, from will always be a non-zero address in _rebalanceInterestWithdrawn(), making the check redundant.
if (to != address(0)) { _withdrawInterest(id, from); _withdrawInterest(id, to); }
#0 - ritik99
2022-04-12T18:57:51Z
All suggestions are valid.