Platform: Code4rena
Start Date: 09/12/2021
Pot Size: $50,000 USDC
Total HM: 19
Participants: 21
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 61
League: ETH
Rank: 2/21
Findings: 6
Award: $6,999.92
🌟 Selected for report: 12
🚀 Solo Findings: 2
🌟 Selected for report: WatchPug
2816.7538 USDC - $2,816.75
WatchPug
The function SavingsAccountUtil.depositFromSavingsAccount() is expected to return the number of equivalent shares for given _asset.
/** * @notice internal function used to get amount of collateral deposited to the pool * @param _fromSavingsAccount if true, collateral is transferred from _sender's savings account, if false, it is transferred from _sender's wallet * @param _toSavingsAccount if true, collateral is transferred to pool's savings account, if false, it is withdrawn from _sender's savings account * @param _asset address of the asset to be deposited * @param _amount amount of tokens to be deposited in the pool * @param _poolSavingsStrategy address of the saving strategy used for collateral deposit * @param _depositFrom address which makes the deposit * @param _depositTo address to which the tokens are deposited * @return _sharesReceived number of equivalent shares for given _asset */ function _deposit( bool _fromSavingsAccount, bool _toSavingsAccount, address _asset, uint256 _amount, address _poolSavingsStrategy, address _depositFrom, address _depositTo ) internal returns (uint256 _sharesReceived) { if (_fromSavingsAccount) { _sharesReceived = SavingsAccountUtil.depositFromSavingsAccount( ISavingsAccount(IPoolFactory(poolFactory).savingsAccount()), _depositFrom, _depositTo, _amount, _asset, _poolSavingsStrategy, true, _toSavingsAccount ); } else { _sharesReceived = SavingsAccountUtil.directDeposit( ISavingsAccount(IPoolFactory(poolFactory).savingsAccount()), _depositFrom, _depositTo, _amount, _asset, _toSavingsAccount, _poolSavingsStrategy ); } }
However, since savingsAccountTransfer() does not return the result of _savingsAccount.transfer(), but returned _amount instead, which means that SavingsAccountUtil.depositFromSavingsAccount() may not return the actual shares (when pps is not 1).
function depositFromSavingsAccount( ISavingsAccount _savingsAccount, address _from, address _to, uint256 _amount, address _token, address _strategy, bool _withdrawShares, bool _toSavingsAccount ) internal returns (uint256) { if (_toSavingsAccount) { return savingsAccountTransfer(_savingsAccount, _from, _to, _amount, _token, _strategy); } else { return withdrawFromSavingsAccount(_savingsAccount, _from, _to, _amount, _token, _strategy, _withdrawShares); } }
function savingsAccountTransfer( ISavingsAccount _savingsAccount, address _from, address _to, uint256 _amount, address _token, address _strategy ) internal returns (uint256) { if (_from == address(this)) { _savingsAccount.transfer(_amount, _token, _strategy, _to); } else { _savingsAccount.transferFrom(_amount, _token, _strategy, _from, _to); } return _amount; }
As a result, the recorded _sharesReceived can be wrong.
function _depositCollateral( address _depositor, uint256 _amount, bool _transferFromSavingsAccount ) internal nonReentrant { uint256 _sharesReceived = _deposit( _transferFromSavingsAccount, true, poolConstants.collateralAsset, _amount, poolConstants.poolSavingsStrategy, _depositor, address(this) ); poolVariables.baseLiquidityShares = poolVariables.baseLiquidityShares.add(_sharesReceived); emit CollateralAdded(_depositor, _amount, _sharesReceived); }
Given:
1.212,000 USDC to yearn strategy, received 10,000 share tokens;12,000 USDC from the saving account as collateral; The recorded CollateralAdded got the wrong number: 12000 which should be 10000;cancelPool(), it fails as the recorded collateral shares are more than the actual collateral.As a result, Alice has lost all the 12,000 USDC.
If Alice managed to borrow with the pool, when the loan defaults, the liquidation will also fail, and cause fund loss to the lenders.
Change to:
function savingsAccountTransfer( ISavingsAccount _savingsAccount, address _from, address _to, uint256 _amount, address _token, address _strategy ) internal returns (uint256) { if (_from == address(this)) { return _savingsAccount.transfer(_amount, _token, _strategy, _to); } else { return _savingsAccount.transferFrom(_amount, _token, _strategy, _from, _to); } }
WatchPug
The current implementation of AaveYield.sol is taking AAVE aToken as a share token (eg, cToken and yToken).
However, AAVE's aTokens are quite different from cToken and yToken as it's always 1:1 to the underlying token, and the holder's balance will keep changing as the yield accumulates.
With the current implementation, users won't be able to get any yields for their deposits.
function _depositERC20(address asset, uint256 amount) internal returns (address aToken, uint256 sharesReceived) { aToken = liquidityToken(asset); uint256 aTokensBefore = IERC20(aToken).balanceOf(address(this)); address lendingPool = ILendingPoolAddressesProvider(lendingPoolAddressesProvider).getLendingPool(); //approve collateral to vault IERC20(asset).approve(lendingPool, 0); IERC20(asset).approve(lendingPool, amount); //lock collateral in vault AaveLendingPool(lendingPool).deposit(asset, amount, address(this), referralCode); sharesReceived = IERC20(aToken).balanceOf(address(this)).sub(aTokensBefore); }
10,000 USDC to AaveYield strategy;10,000 USDC has accumulated 2,000 USDC of yields;10,000 USDC.As a result, Alice has lost all the yields earned.
Use the deposited token amount / current total aToken balance as sharesReceived for _depositERC20.
#0 - ritik99
2021-12-27T10:28:03Z
Duplicate of #137
1267.5392 USDC - $1,267.54
WatchPug
function emergencyWithdraw(address _asset, address payable _wallet) external onlyOwner returns (uint256 received) { require(_wallet != address(0), 'cant burn'); uint256 amount = IERC20(_asset).balanceOf(address(this)); IERC20(_asset).safeTransfer(_wallet, received); received = amount; }
received is not being assigned prior to L81, therefore, at L81, received is 0.
As a result, the emergencyWithdraw() does not work, in essence.
Change to:
function emergencyWithdraw(address _asset, address payable _wallet) external onlyOwner returns (uint256 received) { require(_wallet != address(0), 'cant burn'); received = IERC20(_asset).balanceOf(address(this)); IERC20(_asset).safeTransfer(_wallet, received); }
#0 - 0xean
2022-01-21T16:34:21Z
upgrading to High sev based on assets being "lost" directly. IE the emergency function will not work.
3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals). ``
🌟 Selected for report: WatchPug
845.0261 USDC - $845.03
WatchPug
There are ERC20 tokens that charge fee for every transfer() or transferFrom().
In the current implementation, NoYield.sol#lockTokens() assumes that the received amount is the same as the transfer amount, and uses it to calculate sharesReceived amounts.
As a result, in unlockTokens(), later users may not be able to successfully withdraw their tokens, as it may revert at L141 for insufficient balance.
function lockTokens( address user, address asset, uint256 amount ) external payable override onlySavingsAccount nonReentrant returns (uint256 sharesReceived) { require(amount != 0, 'Invest: amount'); if (asset != address(0)) { IERC20(asset).safeTransferFrom(user, address(this), amount); } else { require(msg.value == amount, 'Invest: ETH amount'); } sharesReceived = amount; emit LockedTokens(user, asset, sharesReceived); }
function _unlockTokens(address asset, uint256 amount) internal returns (uint256 received) { require(amount != 0, 'Invest: amount'); received = amount; if (asset == address(0)) { (bool success, ) = savingsAccount.call{value: received}(''); require(success, 'Transfer failed'); } else { IERC20(asset).safeTransfer(savingsAccount, received); } emit UnlockedTokens(asset, received); }
Consider comparing before and after balance to get the actual transferred amount.
🌟 Selected for report: WatchPug
281.6754 USDC - $281.68
WatchPug
function getUniswapLatestPrice(address num, address den) public view returns (uint256, uint256) { bytes32 _poolTokensId = getUniswapPoolTokenId(num, den); address _pool = uniswapPools[_poolTokensId]; if (_pool == address(0)) { return (0, 0); } int24 _twapTick = OracleLibrary.consult(_pool, uniswapPriceAveragingPeriod); uint256 _numTokens = OracleLibrary.getQuoteAtTick(_twapTick, 10**30, num, den); return (_numTokens, 30); }
The PriceOracle.sol contract can not compile due to wrong usage of OracleLibrary.getQuoteAtTick().
Change to:
function getUniswapLatestPrice(address num, address den) public view returns (uint256, uint256) { bytes32 _poolTokensId = getUniswapPoolTokenId(num, den); address _pool = uniswapPools[_poolTokensId]; if (_pool == address(0)) { return (0, 0); } (int24 _twapTick, ) = OracleLibrary.consult(_pool, uniswapPriceAveragingPeriod); uint256 _numTokens = OracleLibrary.getQuoteAtTick(_twapTick, 10**30, num, den); return (_numTokens, 30); }
#0 - ritik99
2021-12-24T07:47:00Z
The contracts do compile, we're just using an older version.
The getQuoteAtTick methods are unchanged from the version we use vs. now.
The change seems to be in the return value of the consult() method: what we use vs. now. Our call still matches their documentation
#1 - 0xean
2022-01-21T00:31:49Z
the contracts certainly compile on my machine based on the repo.
I do think that it is a risk that is worth highlighting that the new oracle library API is incompatible with the contracts, but marking down to low risk based on current documentation from Uniswap.
🌟 Selected for report: sirhashalot
51.3353 USDC - $51.34
WatchPug
There are many functions across the codebase that will perform an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.
Instances include:
IERC20(asset).approve(lendingPool, 0); IERC20(asset).approve(lendingPool, amount);
It is usually good to add a require-statement that checks the return value or to use something like safeApprove; unless one is sure the given token reverts in case of a failure.
#0 - ritik99
2021-12-25T16:56:51Z
We will be going ahead with the recommendations and references provided in #2
51.3353 USDC - $51.34
WatchPug
The initializer function that initializes important contract state can be called by anyone.
The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.
In the best case for the victim, they notice it and have to redeploy their contract costing gas.
Use the constructor to initialize non-proxied contracts.
For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.
#0 - ritik99
2021-12-27T13:32:28Z
Cases of frontrunning initialize are easy to identify since they're only callable once. As mentioned by the warden, the only real issue would be the cost of redeployment. Hence the (0) non-critical label is apt
#1 - 0xean
2022-01-21T01:10:08Z
Upgrading to low-risk as this is really a state handling issue for the deployment
1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.
🌟 Selected for report: WatchPug
281.6754 USDC - $281.68
WatchPug
Given that Pool is deployed as a proxied contract, it should use the Upgradeable variant of OpenZeppelin Contracts.
function _createPool( uint256 _poolSize, uint256 _borrowRate, address _borrowToken, address _collateralToken, uint256 _idealCollateralRatio, uint256 _repaymentInterval, uint256 _noOfRepaymentIntervals, address _poolSavingsStrategy, uint256 _collateralAmount, bool _transferFromSavingsAccount, bytes32 _salt, address _lenderVerifier ) internal { bytes memory data = _encodePoolInitCall( _poolSize, _borrowRate, _borrowToken, _collateralToken, _idealCollateralRatio, _repaymentInterval, _noOfRepaymentIntervals, _poolSavingsStrategy, _collateralAmount, _transferFromSavingsAccount, _lenderVerifier ); bytes32 salt = keccak256(abi.encodePacked(_salt, msg.sender)); bytes memory bytecode = abi.encodePacked(type(SublimeProxy).creationCode, abi.encode(poolImpl, address(0x01), data)); uint256 amount = _collateralToken == address(0) ? _collateralAmount : 0; address pool = _deploy(amount, salt, bytecode); poolRegistry[pool] = true; emit PoolCreated(pool, msg.sender); }
Otherwise, the constructor functions of Pool's parent contracts which may change storage at deploy time, won't work for deployed instances.
The effect may be different for different OpenZeppelin libraries.
Take ReentrancyGuard for example, the code inside ReentrancyGuard.sol#constructor won't work, should use ReentrancyGuardUpgradeable.sol instead:
import '@openzeppelin/contracts/utils/ReentrancyGuard.sol'; import '@openzeppelin/contracts-upgradeable/proxy/Initializable.sol'; import '@openzeppelin/contracts-upgradeable/token/ERC20/ERC20PausableUpgradeable.sol';
contract Pool is Initializable, ERC20PausableUpgradeable, IPool, ReentrancyGuard {
Change to:
import "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; import '@openzeppelin/contracts-upgradeable/proxy/Initializable.sol'; import '@openzeppelin/contracts-upgradeable/token/ERC20/ERC20PausableUpgradeable.sol';
contract Pool is Initializable, ReentrancyGuardUpgradeable, ERC20PausableUpgradeable, IPool {
function initialize( uint256 _borrowAmountRequested, uint256 _borrowRate, address _borrower, address _borrowAsset, address _collateralAsset, uint256 _idealCollateralRatio, uint256 _repaymentInterval, uint256 _noOfRepaymentIntervals, address _poolSavingsStrategy, uint256 _collateralAmount, bool _transferFromSavingsAccount, address _lenderVerifier, uint256 _loanWithdrawalDuration, uint256 _collectionPeriod ) external payable initializer { poolFactory = msg.sender; poolConstants.borrowAsset = _borrowAsset; poolConstants.idealCollateralRatio = _idealCollateralRatio; poolConstants.collateralAsset = _collateralAsset; poolConstants.poolSavingsStrategy = _poolSavingsStrategy; poolConstants.borrowAmountRequested = _borrowAmountRequested; _initialDeposit(_borrower, _collateralAmount, _transferFromSavingsAccount); poolConstants.borrower = _borrower; poolConstants.borrowRate = _borrowRate; poolConstants.noOfRepaymentIntervals = _noOfRepaymentIntervals; poolConstants.repaymentInterval = _repaymentInterval; poolConstants.lenderVerifier = _lenderVerifier; poolConstants.loanStartTime = block.timestamp.add(_collectionPeriod); poolConstants.loanWithdrawalDeadline = block.timestamp.add(_collectionPeriod).add(_loanWithdrawalDuration); __ReentrancyGuard_init(); __ERC20_init('Pool Tokens', 'PT'); try ERC20Upgradeable(_borrowAsset).decimals() returns(uint8 _decimals) { _setupDecimals(_decimals); } catch(bytes memory) {} }
10.9563 USDC - $10.96
WatchPug
It's a best practice to use the latest compiler version.
The specified minimum compiler version is not up to date. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.
List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo
#0 - ritik99
2021-12-26T17:34:19Z
Duplicate of #39
🌟 Selected for report: sirhashalot
Also found by: 0x0x0x, WatchPug, cmichel, pmerkleplant, robee
WatchPug
Redundant code increase contract size and gas usage at deployment.
uint256 _tokenInStrategy = _liquidityShares; _tokenInStrategy = IYield(_strategyList[index]).getTokensForShares(_liquidityShares, _collateralAsset);
L897, _tokenInStrategy = _liquidityShares is redundant.
#0 - ritik99
2022-01-07T22:42:54Z
This is a duplicate of #36 since both deal with unnecessary initializations. The location of the issue mentioned in the two places is however different
22.5438 USDC - $22.54
WatchPug
Using ++i is more gas efficient than i++, especially in a loop.
For example:
for (uint256 _index = 0; _index < _strategyList.length; _index++) {
#0 - ritik99
2021-12-25T18:43:00Z
Duplicate of #22
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
Based on the current implementation, shares are in aToken, which is 1:1 to the underlying token.
Therefore, getTokensForShares() and getSharesForTokens() can be much simplier.
function getTokensForShares(uint256 shares, address asset) public view override returns (uint256 amount) { if (shares == 0) return 0; address aToken = liquidityToken(asset); (, , , , , , , uint256 liquidityIndex, , ) = IProtocolDataProvider(protocolDataProvider).getReserveData(asset); amount = IScaledBalanceToken(aToken).scaledBalanceOf(address(this)).mul(liquidityIndex).mul(shares).div( IERC20(aToken).balanceOf(address(this)) ); }
can be changed to:
function getTokensForShares(uint256 shares, address asset) public view override returns (uint256) { return shares; }
function getSharesForTokens(uint256 amount, address asset) external view override returns (uint256 shares) { shares = (amount.mul(1e18)).div(getTokensForShares(1e18, asset)); }
can be changed to:
function getSharesForTokens(uint256 amount, address asset) external view override returns (uint256) { return amount; }
#0 - ritik99
2021-12-24T07:28:51Z
Instead of making the proposed changes, we plan to use Aave's wrappers for aTokens so that it conforms with the behaviour of other LP tokens
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
Check if _poolStatus and block.timestamp earlier can avoid unnecessary code execution when this check failed.
function withdrawBorrowedAmount() external override onlyBorrower(msg.sender) nonReentrant { LoanStatus _poolStatus = poolVariables.loanStatus; uint256 _tokensLent = totalSupply(); require( _poolStatus == LoanStatus.COLLECTION && poolConstants.loanStartTime < block.timestamp && block.timestamp < poolConstants.loanWithdrawalDeadline, 'WBA1' ); IPoolFactory _poolFactory = IPoolFactory(poolFactory); require(_tokensLent >= _poolFactory.minBorrowFraction().mul(poolConstants.borrowAmountRequested).div(10**30), 'WBA2'); poolVariables.loanStatus = LoanStatus.ACTIVE; uint256 _currentCollateralRatio = getCurrentCollateralRatio(); require(_currentCollateralRatio >= poolConstants.idealCollateralRatio, 'WBA3'); uint256 _noOfRepaymentIntervals = poolConstants.noOfRepaymentIntervals; uint256 _repaymentInterval = poolConstants.repaymentInterval; IRepayment(_poolFactory.repaymentImpl()).initializeRepayment( _noOfRepaymentIntervals, _repaymentInterval, poolConstants.borrowRate, poolConstants.loanStartTime, poolConstants.borrowAsset ); IExtension(_poolFactory.extension()).initializePoolExtension(_repaymentInterval); address _borrowAsset = poolConstants.borrowAsset; (uint256 _protocolFeeFraction, address _collector) = _poolFactory.getProtocolFeeData(); uint256 _protocolFee = _tokensLent.mul(_protocolFeeFraction).div(10**30); delete poolConstants.loanWithdrawalDeadline; uint256 _feeAdjustedWithdrawalAmount = _tokensLent.sub(_protocolFee); SavingsAccountUtil.transferTokens(_borrowAsset, _protocolFee, address(this), _collector); SavingsAccountUtil.transferTokens(_borrowAsset, _feeAdjustedWithdrawalAmount, address(this), msg.sender); emit AmountBorrowed(_feeAdjustedWithdrawalAmount, _protocolFee); }
Change to:
function withdrawBorrowedAmount() external override onlyBorrower(msg.sender) nonReentrant { LoanStatus _poolStatus = poolVariables.loanStatus; require( _poolStatus == LoanStatus.COLLECTION && poolConstants.loanStartTime < block.timestamp && block.timestamp < poolConstants.loanWithdrawalDeadline, 'WBA1' ); uint256 _tokensLent = totalSupply(); IPoolFactory _poolFactory = IPoolFactory(poolFactory); require(_tokensLent >= _poolFactory.minBorrowFraction().mul(poolConstants.borrowAmountRequested).div(10**30), 'WBA2'); poolVariables.loanStatus = LoanStatus.ACTIVE; uint256 _currentCollateralRatio = getCurrentCollateralRatio(); require(_currentCollateralRatio >= poolConstants.idealCollateralRatio, 'WBA3'); uint256 _noOfRepaymentIntervals = poolConstants.noOfRepaymentIntervals; uint256 _repaymentInterval = poolConstants.repaymentInterval; IRepayment(_poolFactory.repaymentImpl()).initializeRepayment( _noOfRepaymentIntervals, _repaymentInterval, poolConstants.borrowRate, poolConstants.loanStartTime, poolConstants.borrowAsset ); IExtension(_poolFactory.extension()).initializePoolExtension(_repaymentInterval); address _borrowAsset = poolConstants.borrowAsset; (uint256 _protocolFeeFraction, address _collector) = _poolFactory.getProtocolFeeData(); uint256 _protocolFee = _tokensLent.mul(_protocolFeeFraction).div(10**30); delete poolConstants.loanWithdrawalDeadline; uint256 _feeAdjustedWithdrawalAmount = _tokensLent.sub(_protocolFee); SavingsAccountUtil.transferTokens(_borrowAsset, _protocolFee, address(this), _collector); SavingsAccountUtil.transferTokens(_borrowAsset, _feeAdjustedWithdrawalAmount, address(this), msg.sender); emit AmountBorrowed(_feeAdjustedWithdrawalAmount, _protocolFee); }
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
function _updateLiquidatorRewardFraction(uint256 _rewardFraction) internal { require(_rewardFraction <= 10**30, 'Fraction has to be less than 1'); liquidatorRewardFraction = _rewardFraction; emit LiquidationRewardFractionUpdated(_rewardFraction); }
Can be changed to:
function _updateLiquidatorRewardFraction(uint256 _rewardFraction) internal { require(_rewardFraction <= 1e30, 'Fraction has to be less than 1'); liquidatorRewardFraction = _rewardFraction; emit LiquidationRewardFractionUpdated(_rewardFraction); }
and save some gas from unnecessary arithmetic operation in 10**30.
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
function updateinterestAccruedTillLastPrincipalUpdate(uint256 _id) internal { require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, 'CreditLine: The credit line is not yet active.'); uint256 _interestAccrued = calculateInterestAccrued(_id); uint256 _newInterestAccrued = (creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate).add(_interestAccrued); creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate = _newInterestAccrued; }
function borrow(uint256 _id, uint256 _amount) external payable nonReentrant onlyCreditLineBorrower(_id) { require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, 'CreditLine: The credit line is not yet active.'); uint256 _borrowableAmount = calculateBorrowableAmount(_id); require(_amount <= _borrowableAmount, "CreditLine::borrow - The current collateral ratio doesn't allow to withdraw the amount"); address _borrowAsset = creditLineConstants[_id].borrowAsset; address _lender = creditLineConstants[_id].lender; updateinterestAccruedTillLastPrincipalUpdate(_id); creditLineVariables[_id].principal = creditLineVariables[_id].principal.add(_amount); creditLineVariables[_id].lastPrincipalUpdateTime = block.timestamp; uint256 _tokenDiffBalance; if (_borrowAsset != address(0)) { uint256 _balanceBefore = IERC20(_borrowAsset).balanceOf(address(this)); _withdrawBorrowAmount(_borrowAsset, _amount, _lender); uint256 _balanceAfter = IERC20(_borrowAsset).balanceOf(address(this)); _tokenDiffBalance = _balanceAfter.sub(_balanceBefore); } else { uint256 _balanceBefore = address(this).balance; _withdrawBorrowAmount(_borrowAsset, _amount, _lender); uint256 _balanceAfter = address(this).balance; _tokenDiffBalance = _balanceAfter.sub(_balanceBefore); } uint256 _protocolFee = _tokenDiffBalance.mul(protocolFeeFraction).div(10**30); _tokenDiffBalance = _tokenDiffBalance.sub(_protocolFee); if (_borrowAsset == address(0)) { (bool feeSuccess, ) = protocolFeeCollector.call{value: _protocolFee}(''); require(feeSuccess, 'Transfer fail'); (bool success, ) = msg.sender.call{value: _tokenDiffBalance}(''); require(success, 'Transfer fail'); } else { IERC20(_borrowAsset).safeTransfer(protocolFeeCollector, _protocolFee); IERC20(_borrowAsset).safeTransfer(msg.sender, _tokenDiffBalance); } emit BorrowedFromCreditLine(_id, _tokenDiffBalance); }
updateinterestAccruedTillLastPrincipalUpdate() is unnecessary as it's being used only once. Therefore it can be inlined in borrow() to make the code simpler and save gas.
Change to:
function borrow(uint256 _id, uint256 _amount) external payable nonReentrant onlyCreditLineBorrower(_id) { require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, 'CreditLine: The credit line is not yet active.'); uint256 _borrowableAmount = calculateBorrowableAmount(_id); require(_amount <= _borrowableAmount, "CreditLine::borrow - The current collateral ratio doesn't allow to withdraw the amount"); address _borrowAsset = creditLineConstants[_id].borrowAsset; address _lender = creditLineConstants[_id].lender; creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate = (creditLineVariables[_id].interestAccruedTillLastPrincipalUpdate).add(calculateInterestAccrued(_id)); creditLineVariables[_id].principal = creditLineVariables[_id].principal.add(_amount); creditLineVariables[_id].lastPrincipalUpdateTime = block.timestamp; uint256 _tokenDiffBalance; if (_borrowAsset != address(0)) { uint256 _balanceBefore = IERC20(_borrowAsset).balanceOf(address(this)); _withdrawBorrowAmount(_borrowAsset, _amount, _lender); uint256 _balanceAfter = IERC20(_borrowAsset).balanceOf(address(this)); _tokenDiffBalance = _balanceAfter.sub(_balanceBefore); } else { uint256 _balanceBefore = address(this).balance; _withdrawBorrowAmount(_borrowAsset, _amount, _lender); uint256 _balanceAfter = address(this).balance; _tokenDiffBalance = _balanceAfter.sub(_balanceBefore); } uint256 _protocolFee = _tokenDiffBalance.mul(protocolFeeFraction).div(10**30); _tokenDiffBalance = _tokenDiffBalance.sub(_protocolFee); if (_borrowAsset == address(0)) { (bool feeSuccess, ) = protocolFeeCollector.call{value: _protocolFee}(''); require(feeSuccess, 'Transfer fail'); (bool success, ) = msg.sender.call{value: _tokenDiffBalance}(''); require(success, 'Transfer fail'); } else { IERC20(_borrowAsset).safeTransfer(protocolFeeCollector, _protocolFee); IERC20(_borrowAsset).safeTransfer(msg.sender, _tokenDiffBalance); } emit BorrowedFromCreditLine(_id, _tokenDiffBalance); }
#0 - ritik99
2021-12-24T07:22:31Z
We believe the code separation helps with readability
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
wethGateway in AaveYield#_withdrawETH()
🌟 Selected for report: WatchPug
83.4956 USDC - $83.50
WatchPug
function borrow(uint256 _id, uint256 _amount) external payable nonReentrant onlyCreditLineBorrower(_id) { require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, 'CreditLine: The credit line is not yet active.'); uint256 _borrowableAmount = calculateBorrowableAmount(_id); require(_amount <= _borrowableAmount, "CreditLine::borrow - The current collateral ratio doesn't allow to withdraw the amount"); address _borrowAsset = creditLineConstants[_id].borrowAsset; address _lender = creditLineConstants[_id].lender;
_borrowableAmount is unnecessary. The code above can be changed to:
function borrow(uint256 _id, uint256 _amount) external payable nonReentrant onlyCreditLineBorrower(_id) { require(creditLineVariables[_id].status == CreditLineStatus.ACTIVE, 'CreditLine: The credit line is not yet active.'); require(_amount <= calculateBorrowableAmount(_id), "CreditLine::borrow - The current collateral ratio doesn't allow to withdraw the amount"); address _borrowAsset = creditLineConstants[_id].borrowAsset; address _lender = creditLineConstants[_id].lender;
function calculateInterest( uint256 _principal, uint256 _borrowRate, uint256 _timeElapsed ) public pure returns (uint256) { uint256 _interest = _principal.mul(_borrowRate).mul(_timeElapsed).div(10**30).div(YEAR_IN_SECONDS); return _interest; }
_interest is unnecessary.
WatchPug
For the arithmetic operations that will never over/underflow, using SafeMath will cost more gas.
For example:
if (_maxPossible > _currentDebt) { return _maxPossible.sub(_currentDebt); }
_maxPossible - _currentDebt will never underflow.
Change to:
if (_maxPossible > _currentDebt) { return _maxPossible - _currentDebt; }
#0 - ritik99
2021-12-26T17:26:56Z
Duplicate of #92
🌟 Selected for report: 0xngndev
Also found by: Jujic, WatchPug, robee, sirhashalot
10.9563 USDC - $10.96
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
require(poolRegistry[msg.sender], 'PoolFactory::onlyPool - Only pool can destroy itself');
require( IVerification(userRegistry).isUser(msg.sender, _verifier), 'PoolFactory::onlyBorrower - Only a valid Borrower can create Pool' );
require(_borrowToken != _collateralToken, 'PoolFactory::createPool - cant borrow the asset put in as collateralToken');
require(_amount <= _borrowableAmount, "CreditLine::borrow - The current collateral ratio doesn't allow to withdraw the amount");
require(_amount != 0, 'SavingsAccount::switchStrategy Amount must be greater than zero');
#0 - ritik99
2021-12-25T16:43:44Z
Duplicate of #47
🌟 Selected for report: 0x0x0x
Also found by: WatchPug, pmerkleplant, robee
15.2171 USDC - $15.22
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
CreditLine.sol#calculateTotalCollateralTokens()
CreditLine.sol#_withdrawBorrowAmount()
#0 - ritik99
2021-12-25T18:37:43Z
Duplicate of #157
🌟 Selected for report: TomFrenchBlockchain
Also found by: WatchPug
WatchPug
function accept(uint256 _id) external { require( creditLineVariables[_id].status == CreditLineStatus.REQUESTED, 'CreditLine::acceptCreditLineLender - CreditLine is already accepted' ); bool _requestByLender = creditLineConstants[_id].requestByLender; require( (msg.sender == creditLineConstants[_id].borrower && _requestByLender) || (msg.sender == creditLineConstants[_id].lender && !_requestByLender), "Only Borrower or Lender who hasn't requested can accept" ); creditLineVariables[_id].status = CreditLineStatus.ACTIVE; emit CreditLineAccepted(_id); }
L599-602 can use ternary operator to make the code simpler and save some gas.
Change to:
function accept(uint256 _id) external { require( creditLineVariables[_id].status == CreditLineStatus.REQUESTED, 'CreditLine::acceptCreditLineLender - CreditLine is already accepted' ); require( msg.sender == (creditLineConstants[_id].requestByLender ? creditLineConstants[_id].borrower : creditLineConstants[_id].lender), "Only Borrower or Lender who hasn't requested can accept" ); creditLineVariables[_id].status = CreditLineStatus.ACTIVE; emit CreditLineAccepted(_id); }
#0 - ritik99
2021-12-26T17:32:55Z
Duplicate of #77