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: 4/24
Findings: 3
Award: $2,463.71
🌟 Selected for report: 0
🚀 Solo Findings: 0
While calculating the amount of shares to withdraw in LenderPool.terminate, _principalWithdrawable (which is amount of tokens) is added to _totalInterestInShares, and then passed to SAVINGS_ACCOUNT.withdrawShares. If tokens : shares are not 1 : 1, which is the norm for yields, incorrect amounts will be withdrawn. Since one share is usually worth more than one token, the incorrect amount will usually be higher than the intended amount, and result in stolen funds from other pools.
It is easy to see that _principalWithdrawable is amount of tokens and _totalInterestInShares is amount of shares in the code below.
address _borrowAsset = pooledCLConstants[_id].borrowAsset; uint256 _borrowedTokens = pooledCLConstants[_id].borrowLimit; uint256 _notBorrowed = _borrowedTokens.sub(POOLED_CREDIT_LINE.getPrincipal(_id)); uint256 _notBorrowedInShares = IYield(_strategy).getSharesForTokens(_notBorrowed, _borrowAsset); uint256 _sharesHeld = pooledCLVariables[_id].sharesHeld; uint256 _totalInterestInShares = _sharesHeld.sub(_notBorrowedInShares); uint256 _principalWithdrawable = _notBorrowed.mul(totalSupply[_id]).div(_borrowedTokens); SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _principalWithdrawable.add(_totalInterestInShares), false);
Under any scenario, it is unreasonable to mix calculation of both, and for the specific case here, _principalWithdrawable must be converted to the amount of shares for the withdrawal to work correctly.
vim, ganache-cli
Convert _principalWithdrawable to shares before withdrawing. Or honestly, just withdraw all _sharesHeld in the pool and avoid those complex calculations altogether.
#0 - ritik99
2022-04-12T12:06:47Z
Duplicate of #21
94.016 USDC - $94.02
We list 6 low-critical findings and 6 non-critical findings here:
PooledCreditLine.request priceOracle fallback to uniswap, and anyone can register uniswap for arbitrary tokens, it is possible to use malicious tokenstype(uint256).maxIn summary of recommended security practices, it's better to restrict function access, use 3rd party libraries for security (e.g. safeTransfer, ReentrancyGuardUpgradeable), and take consideration of status.
LenderPool.withdrawInterest allows lenders to retrieve the interest that is owed to them. However it doesn’t check the weather msg.sender is lender. Which means an attacker can call LenderPool.withdrawInterest in order to make others retrieve their interest. It could be harmless, the attacker can get any interest. But the lenders may not want to retrieve their interest at that moment.
Use msg.sender as lender.
If _status == PooledCreditLineStatus.CANCELLED, LenderPool.calculatePrincipalWithdrawable returns 0. But it is not the correct number.
Take PooledCreditLineStatus.CANCELLED into consideration.
Add _status == PooledCreditLineStatus.CANCELLED into the statement of if condition.
PooledCreditLine.request priceOracle fallback to uniswap, and anyone can register uniswap for arbitrary tokens, it is possible to use malicious tokensCheck on borrow/collateral asset relies on IPriceOracle(priceOracle).doesFeedExist(_request.borrowAsset, _request.collateralAsset). However, since priceOracle falls back to uniswap, and anyone can register uniswap for arbitrary tokens, it is possible to use malicious tokens.
While lenders should probably be sensible enough to avoid those kinds of pools, it is hard to judge the difficulty of telling between benign uniswap pair and potentially malicious uniswap pair (most users probably just check name).
Use whitelist to prevent malicious uniswap pair.
If ERC20 transfer fails, it will return false rather than revert, and lose _fee of _to address.
Use safeTransfer to check the return value:
IERC20(_borrowAsset).safeTransfer(_to, _fee);
The variable uint256 _principalPaid is uninitialized. In the case here, solc-bin implicitly sets uninitialized stack value to 0, but this feature is undocumented and not guaranteed in solc, thus it would be best to do an explicit initialization here.
If _amount <= _interestToPay, _principalPaid will be uninitialized.
Initialize _principalPaid to 0:
uint256 _principalPaid = 0;
start function in LenderPool does not check _to, so anyone can call this function and claim the _fee.
It’s better to limit the caller who is involved in.
PooledCreditLine.repay can be called by any user. It doesn’t check whether msg.sender is who should repay. However it won’t cause any problem. Also it might be intended.
Check the relationship between msg.sender and PooledCreditLine.
type(uint256).maxIn create function of LenderPool, it always approves type(uint256).max amount.
It’s better to use a limited amount rather than type(uint256).max.
_beforeTokenTransfer has an ids array, it’s better to use continue in the for loop rather than return.
if (from == address(0)) { continue; }
Use a non upgradeable version of reentrancy guard will not initialize when using an upgradable contract. Also it will not reserve empty space to allow future versions.
https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L25 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L25
Use ReentrancyGuardUpgradeable: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/security/ReentrancyGuardUpgradeable.sol
The variable _clc.collateralAssetStrategy is assigned twice in _createRequest function of PooledCreditLine.
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
Just assign once.
Using constructor in upgradable contract is not upgrade safe. It will cause an error when deploying an upgradable contract by hardhat: Error: Contract XXXXX is not upgrade safe.
https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L592-L595 https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L205-L213
It’s better to put immutable variables into initialize function.
#0 - ritik99
2022-04-13T10:33:36Z
#1 - HardlyDifficult
2022-04-18T15:20:25Z
38.1576 USDC - $38.16
They can be declared private for gas savings, only creating view functions for external read.
Declare these variables to private.
#0 - ritik99
2022-04-12T19:03:32Z
Suggestion is valid