Omniscia Steadefi Audit

LendingPool Manual Review Findings

LendingPool Manual Review Findings

LPL-01M: Improper Execution Flows

TypeSeverityLocation
Language SpecificLendingPool.sol:L248, L296

Description:

The LendingPool::borrow and LendingPool::repay functions do not implement correct execution flows and thus contain incorrect system states when they perform outward / inward transfer of funds.

Impact:

While the contract is adequately protected against re-entrancy attacks, other system modules that rely on LendingPool data points may be prone to cross-contract re-entrancy attacks as a result of the incorrect system state that LendingPool has during these transfers.

Example:

contracts/lending/LendingPool.sol
236/**
237  * Borrow asset from lending pool, adding debt
238  * @param _borrowAmount Amount of tokens to borrow in token decimals
239*/
240function borrow(uint256 _borrowAmount) external nonReentrant whenNotPaused onlyBorrower {
241  require(_borrowAmount > 0, "Amount must be > 0");
242  require(_borrowAmount <= totalAvailableSupply(), "Not enough lending liquidity to borrow");
243

244  // Update pool with accrued interest and latest timestamp
245  _updatePoolWithInterestsAndTimestamp(0);
246

247  // Transfer borrowed token from pool to manager
248  IERC20(asset).safeTransfer(msg.sender, _borrowAmount);
249

250  // Calculate debt amount
251  uint256 debt = totalBorrows == 0 ? _borrowAmount : _borrowAmount * totalBorrowDebt / totalBorrows;
252

253  // Update pool state
254  totalBorrows = totalBorrows + _borrowAmount;
255  totalBorrowDebt = totalBorrowDebt + debt;
256

257  // Update borrower state
258  borrowers[msg.sender].debt = borrowers[msg.sender].debt + debt;
259  borrowers[msg.sender].lastUpdatedAt = block.timestamp;
260

261  emit Borrow(msg.sender, debt, _borrowAmount);
262}

Recommendation:

We advise the LendingPool::borrow function to transfer the _borrowAmount funds outward at the end of its function execution and the LendingPool::repay function to transfer the _repayAmount inward at the start of the function after the proper _repayAmount has been assessed, ensuring that the LendingPool contract contains a correct state when the asset transfer is being performed.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The recommended course of action for the two referenced instances has been applied to the letter, ensuring that the LendingPool storage state during inward and outward token transfers is correct.

LPL-02M: Improper Enforcement of Access Control

TypeSeverityLocation
Logical FaultLendingPool.sol:L268

Description:

The LendingPool::repay function improperly mandates that the caller is a borrower via the onlyBorrower modifier.

Impact:

If a borrower is removed from the system while their debt is still active, they will be unable to repay it thus causing bad debt to be created in the system undesirably as the ex-borrower may wish to actually repay it.

Example:

contracts/lending/LendingPool.sol
236/**
237  * Borrow asset from lending pool, adding debt
238  * @param _borrowAmount Amount of tokens to borrow in token decimals
239*/
240function borrow(uint256 _borrowAmount) external nonReentrant whenNotPaused onlyBorrower {
241  require(_borrowAmount > 0, "Amount must be > 0");
242  require(_borrowAmount <= totalAvailableSupply(), "Not enough lending liquidity to borrow");
243

244  // Update pool with accrued interest and latest timestamp
245  _updatePoolWithInterestsAndTimestamp(0);
246

247  // Transfer borrowed token from pool to manager
248  IERC20(asset).safeTransfer(msg.sender, _borrowAmount);
249

250  // Calculate debt amount
251  uint256 debt = totalBorrows == 0 ? _borrowAmount : _borrowAmount * totalBorrowDebt / totalBorrows;
252

253  // Update pool state
254  totalBorrows = totalBorrows + _borrowAmount;
255  totalBorrowDebt = totalBorrowDebt + debt;
256

257  // Update borrower state
258  borrowers[msg.sender].debt = borrowers[msg.sender].debt + debt;
259  borrowers[msg.sender].lastUpdatedAt = block.timestamp;
260

261  emit Borrow(msg.sender, debt, _borrowAmount);
262}
263

264/**
265  * Repay asset to lending pool, reducing debt
266  * @param _repayAmount Amount of debt to repay in token decimals
267*/
268function repay(uint256 _repayAmount) external nonReentrant whenNotPaused onlyBorrower {
269  require(_repayAmount > 0, "Amount must be > 0");
270  require(maxRepay(msg.sender) > 0, "Repay amount must be > 0");
271

272  if (_repayAmount > maxRepay(msg.sender)) {
273    _repayAmount = maxRepay(msg.sender);
274  }
275

276  // Update pool with accrued interest and latest timestamp
277  _updatePoolWithInterestsAndTimestamp(0);
278

279  uint256 borrowerTotalRepayAmount = maxRepay(msg.sender);
280

281  // Calculate debt to reduce based on repay amount
282  uint256 debt = _repayAmount * SAFE_MULTIPLIER
283                              / borrowerTotalRepayAmount
284                              * borrowers[msg.sender].debt
285                              / SAFE_MULTIPLIER;
286

287  // Update pool state
288  totalBorrows = totalBorrows - _repayAmount;
289  totalBorrowDebt = totalBorrowDebt - debt;
290

291  // Update borrower state
292  borrowers[msg.sender].debt = borrowers[msg.sender].debt - debt;
293  borrowers[msg.sender].lastUpdatedAt = block.timestamp;
294

295  // Transfer repay tokens to the pool
296  IERC20(asset).safeTransferFrom(msg.sender, address(this), _repayAmount);
297

298  emit Repay(msg.sender, debt, _repayAmount);
299}

Recommendation:

We advise the LendingPool::repay function to no longer apply the modifier as a non-zero LendingPool::maxRepay value indicates that a LendingPool::borrow transaction was performed that has applied the borrower check. Additionally, if a borrower is removed from the system they should be able to repay their debt regardless.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The LendingPool::repay function no longer mandates that its caller is a borrower, preventing the behaviour outlined in the exhibit and thus alleviating it.

LPL-03M: Improper Native Deposit / Withdrawal Handling System

TypeSeverityLocation
Logical FaultLendingPool.sol:L187-L196, L211-L218

Description:

The LendingPool::deposit and LendingPool::withdraw functions will improperly handle incoming native payments and redundantly contain an _isNative variable. In detail, if a user submits a LendingPool::deposit call with a non-zero msg.value but a false value for _isNative, their funds will be locked in the contract forever.

Impact:

It is presently possible to permanently lock native funds within the contract, rendering them irrecoverable.

Example:

contracts/lending/LendingPool.sol
181/**
182  * Deposits asset into lending pool and mint ibToken to user
183  * @param _assetAmount Amount of asset tokens to deposit in token decimals
184  * @param _isNative Boolean for whether asset is a native token or not
185*/
186function deposit(uint256 _assetAmount, bool _isNative) external payable nonReentrant whenNotPaused {
187  if (_isNative) {
188    require(isNativeAsset == true, "Lending pool asset not native token");
189    require(msg.value > 0, "Quantity must be > 0");
190    require(_assetAmount == msg.value, "Amount != msg.value");
191

192    IWAVAX(asset).deposit{ value: msg.value }();
193  } else {
194    require(_assetAmount > 0, "Deposited amount must be > 0");
195    IERC20(asset).safeTransferFrom(msg.sender, address(this), _assetAmount);
196  }
197

198  // Update pool with accrued interest and latest timestamp
199  _updatePoolWithInterestsAndTimestamp(_assetAmount);
200

201  uint256 sharesAmount = _mintShares(_assetAmount);
202

203  emit Deposit(msg.sender, sharesAmount, _assetAmount);
204}

Recommendation:

We advise the code to remove the _isNative variable altogether, instead evaluating whether msg.value is non-zero in LendingPool::deposit and evaluating isNativeAsset in LendingPool::withdraw.

In the LendingPool::deposit function, the isNativeAsset value should be mandated as true and the msg.value to be equivalent to _assetAmount when it is non-zero. Alternatively (i.e. else branch), the isNativeAsset value should be mandated as false and the _assetAmount as non-zero.

The withdraw function does not require the _isNative variable as the isNativeAsset variable is sufficient to assess which logic branches of it should be executed.

These changes will minimize the gas cost involved in all functions and will resolve the present vulnerability outlined in the description of this issue.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The LendingPool::deposit and LendingPool::withdraw flows were altered per our recommendation, making use of the isNativeAsset contract-level variable and preventing the lock of native funds in the contract due to misuse of these functions.

LPL-04M: Inexistent Slippage Protections

TypeSeverityLocation
Logical FaultLendingPool.sol:L186, L210

Description:

The LendingPool::deposit and LendingPool::withdraw functions do not apply any form of slippage protection, rendering all depositors and withdrawers prone to slippage related attacks (such as sandwich attacks).

Impact:

The current mechanisms of the contract render the users prone to arbitrage attacks, causing them to receive less shares and less funds than they expect from their deposits and withdrawals.

Example:

contracts/lending/LendingPool.sol
181/**
182  * Deposits asset into lending pool and mint ibToken to user
183  * @param _assetAmount Amount of asset tokens to deposit in token decimals
184  * @param _isNative Boolean for whether asset is a native token or not
185*/
186function deposit(uint256 _assetAmount, bool _isNative) external payable nonReentrant whenNotPaused {
187  if (_isNative) {
188    require(isNativeAsset == true, "Lending pool asset not native token");
189    require(msg.value > 0, "Quantity must be > 0");
190    require(_assetAmount == msg.value, "Amount != msg.value");
191

192    IWAVAX(asset).deposit{ value: msg.value }();
193  } else {
194    require(_assetAmount > 0, "Deposited amount must be > 0");
195    IERC20(asset).safeTransferFrom(msg.sender, address(this), _assetAmount);
196  }
197

198  // Update pool with accrued interest and latest timestamp
199  _updatePoolWithInterestsAndTimestamp(_assetAmount);
200

201  uint256 sharesAmount = _mintShares(_assetAmount);
202

203  emit Deposit(msg.sender, sharesAmount, _assetAmount);
204}
205

206/**
207  * Withdraws asset from lending pool, burns ibToken from user
208  * @param _ibTokenAmount Amount of ibTokens to burn in 1e18
209*/
210function withdraw(uint256 _ibTokenAmount, bool _isNative) external nonReentrant whenNotPaused {
211  if (_isNative) {
212    require(isNativeAsset == true, "Lending pool asset not native token");
213    require(_ibTokenAmount > 0, "Amount must be > 0");
214    require(_ibTokenAmount <= balanceOf(msg.sender), "Withdraw amount exceeds balance");
215  } else {
216    require(_ibTokenAmount > 0, "Amount must be > 0");
217    require(_ibTokenAmount <= balanceOf(msg.sender), "Withdraw amount exceeds balance");
218  }
219

220  // Update pool with accrued interest and latest timestamp
221  _updatePoolWithInterestsAndTimestamp(0);
222

223  uint256 withdrawAmount = _burnShares(_ibTokenAmount);
224

225  if (_isNative) {
226    IWAVAX(asset).withdraw(withdrawAmount);
227    (bool success, ) = msg.sender.call{value: withdrawAmount}("");
228    require(success, "Transfer failed.");
229  } else {
230    IERC20(asset).safeTransfer(msg.sender, withdrawAmount);
231  }
232

233  emit Withdraw(msg.sender, _ibTokenAmount, withdrawAmount);
234}

Recommendation:

We advise both functions to introduce a new variable representing the minimum sharesAmount and minimum withdrawAmount respectively that they wish to accept as a result of their LendingPool::deposit / LendingPool::withdraw transaction.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The LendingPool::deposit and LendingPool::withdraw functions were updated to accept an additional argument that represents the minimum shares / withdrawal amount the user is willing to accept, enabling them to control the slippage they expose themselves to.

LPL-05M: Total Fund Loss of First Deposit

TypeSeverityLocation
Logical FaultLendingPool.sol:L310, L311

Description:

The LendingPool::_mintShares function follows an unconventional paradigm whereby the previousTotalValue of the contract is evaluated as an indicator of existing shares rather than the actual ERC20::totalSupply. As a result, it is possible to race the first LendingPool::deposit transaction and transmit asset funds directly to the contract. This will cause the previousTotalValue to evaluate to a non-zero value due to how LendingPool::totalValue -> LendingPool::totalAvailableSupply evaluates the balanceOf the contract itself. As such, the LendingPool::_mintShares function will calculate _assetAmount * totalSupply() / previousTotalValue with a zero totalSupply, causing the transaction to mint zero shares.

Impact:

The present calculation enables a malicious party to transmit dust to the LendingPool implementation of a particular asset and cause total fund loss for the depositor.

Example:

contracts/lending/LendingPool.sol
303/**
304  * Calculate amount of ibTokens owed to depositor and mints them
305  * @param _assetAmount  Amount of tokens to deposit in token decimals
306  * @return shares  Amount of ibTokens minted in 1e18
307*/
308function _mintShares(uint256 _assetAmount) internal returns (uint256) {
309  // Calculate liquidity share amount
310  uint256 previousTotalValue = totalValue() - _assetAmount;
311  uint256 shares = previousTotalValue == 0 ? _assetAmount * _to18ConversionFactor(asset) : _assetAmount * totalSupply() / previousTotalValue;
312

313  // Mint ibToken to user equal to liquidity share amount
314  _mint(msg.sender, shares);
315

316  return shares;
317}

Recommendation:

We advise the contract to utilize the totalSupply function as an indicator of whether shares exist in the system instead of a previousTotalValue evaluation.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The share calculation mechanism was adjusted to utilize the ERC20::totalSupply per our recommendation, preventing the first deposit from being susceptible to attacks.

Prev

JoeOracle.sol (JOE-M)

Next

LendingPoolConfig.sol (LPC-M)

On this page