Sublime contest - pmerkleplant's results

Handle

pmerkleplant

Vulnerability details

Impact

ETH deposited by a user through the Pool.depositCollateral function could be unaccounted as collateral and therefore lost for the user.

Proof of Concept

If a user calls the Pool.depositCollateral function with _transferFromSavingsAccount = true and some amount, the function Pool._deposit will be called with _asset = poolConstants.collateralAsset.

From there SavingsAccountUtil.depositFromSavingsAccount is called, and as _toSavingsAccount = true, from there the function SavingsAccountUtil.savingsAccountTransfer (see line 22).

In SavingsAccountUtil.savingsAccountTransfer the asset, i.e. poolConstants.collateralAsset, is fetched from the user's savings account to the pool's savings account.

However, if the user sends ETH to the Pool.depositCollateral function as well, this ETH is not accounted for and will be lost for the user.

Handle

pmerkleplant

Vulnerability details

Impact

If a variable is not initialized, it is assumed to have the default value. Explicitly initalizing a variable with its default value costs unnecessary gas.

For more info, see Mudit Gupta's Blog at point "No need to initialize variables with default values".

Therefore, following variable declarations could be refactored:

./CreditLine/CreditLine.sol:484: for (uint256 _index = 0; _index < _strategyList.length; _index++)
./CreditLine/CreditLine.sol:662: for (uint256 _index = 0; _index < _strategyList.length; _index++)
./CreditLine/CreditLine.sol:738: for (uint256 _index = 0; _index < _strategyList.length; _index++)
./CreditLine/CreditLine.sol:812: uint256 _principalPaid = 0;
./CreditLine/CreditLine.sol:892: for (uint256 index = 0; index < _strategyList.length; index++)
./CreditLine/CreditLine.sol:959: for (uint256 index = 0; index < _strategyList.length; index++)
./Pool/Pool.sol:359: uint256 _collateralShares = 0;
./SavingsAccount/SavingsAccount.sol:289: for (uint256 i = 0; i < _strategyList.length; i++)
./SavingsAccount/SavingsAccount.sol:467: for (uint256 i = 0; i < _strategyList.length; i++)

Tools used

grep

Handle

pmerkleplant

Vulnerability details

Impact

The local variables _receivedToken in the functions SavingsAccount.withdraw and SavingsAccount.withdrawFrom are unused.

Removing them would save gas.

Tools used

slither

Handle

pmerkleplant

Vulnerability details

Impact

Caching the array length outside a loop in a variable saves reading it on each iteration as long as the array's length is not changed during the loop.

Therefore, the following loops could be refactored:

CreditLine/CreditLine.sol:484: for (uint256 _index = 0; _index < _strategyList.length; _index++)
CreditLine/CreditLine.sol:662: for (uint256 _index = 0; _index < _strategyList.length; _index++)
CreditLine/CreditLine.sol:738: for (uint256 _index = 0; _index < _strategyList.length; _index++)
CreditLine/CreditLine.sol:892: for (uint256 index = 0; index < _strategyList.length; index++)
CreditLine/CreditLine.sol:959: for (uint256 index = 0; index < _strategyList.length; index++)
SavingsAccount/SavingsAccount.sol:289: for (uint256 i = 0; i < _strategyList.length; i++)
SavingsAccount/SavingsAccount.sol:467: for (uint256 i = 0; i < _strategyList.length; i++)

Recommended Mitigation Steps

Use the following pattern for refactoring:

uint length = arr.length;
for (uint i; i < length; i++) {
    // ...
}

Tools used

grep

Handle

pmerkleplant

Vulnerability details

Using a local variable instead of loading an array element twice circumvents a second array boundary check and saves gas.

Therefore, the following functions could be refactored by using a local variable for the array element:

CreditLine::_transferCollateral
CreditLine::calculateTotalCollateralTokens
CreditLine::_repayFromSavingsAccount
CreditLine::_withdrawBorrowAmount
CreditLine::_depositCollateralFromSavingsAccount (local variable not used in line 487)
SavingsAccount::getTotalTokens
SavingsAccount::withdrawAll