Wild Credit contest - cmichel's results

Handle

cmichel

Vulnerability details

The LendingPair.accrueAccount function distribtues rewards before updating the cumulative supply / borrow indexes as well as the index + balance for the user (by minting supply tokens / debt). This means the percentage of the user's balance to the total is not correct as the total can be updated several times in between.

function accrueAccount(address _account) public {
  // distributes before updating accrual state
  _distributeReward(_account);
  accrue();
  _accrueAccountInterest(_account);

  if (_account != feeRecipient()) {
    _accrueAccountInterest(feeRecipient());
  }
}

Example: Two users deposit the same amounts in the same block. Thus, after some time they should receive the same tokens.

1. User A and B deposit 1000 tokens (in the same block) and are minted 1000 tokens in return. Total supply = 2000
2. Assume after 50,000 blocks, A calls accrueAccount(A) which first calls _distributeReward. A is paid out 1000/2000 = 50% of the 50,000 blocks reward since deposit. Afterwards, accrue + _accrueAccountInterest(A) is called and A is minted 200 more tokens due to supplier lending rate. The supply totalSupply is now 2200.
3. After another 50,000 blocks, A calls accrueAccount(A) again. which first calls _distributeReward. A is paid out 1200/2200 = 54.5454% of the 50,000 blocks reward since deposit.

From here, you can already see that A receives more than 50% of the 100,000 block rewards although they deposited at the same time as B and didn't deposit or withdraw any funds. B will receive ~1000/2200 = 45% (ignoring any new LP supply tokens minted for A's second claim.)

Impact

Wrong rewards will be minted for the users which do not represent their real fair share. Usually, users will get fewer rewards than they should receive as their individual interest was not updated yet but the totals (total debt and total supply) could have been updated by other accounts in between.

Recommended Mitigation Steps

There are two issues that both contribute to it:

• total LP supply and total debt must be updated by the total new interest when accrue is called, not only increased by an individual user's interest. See my other issue "Reward computation is wrong" that goes into more depth
• Lending/borrow accrual must happen before reward distribution

Handle

cmichel

Vulnerability details

The LendingPair.liquidateAccount function does not accrue and update the cumulativeInterestRate first, it only calls _accrueAccountInterest which does not update and instead uses the old cumulativeInterestRate.

Impact

The liquidatee (borrower)'s state will not be up to date. I could skip some interest payments by liquidating myself instead of repaying if I'm under-water. As the market interest index is not accrued, the borrower does not need to pay any interest accrued from the time of the last accrual until now.

Recommendation

It should call accrueAccount instead of _accrueAccountInterest

Handle

cmichel

Vulnerability details

The LendingPair.liquidateAccount function tries to pay out underlying supply tokens to the liquidator using _safeTransfer(IERC20(supplyToken), msg.sender, supplyOutput) but there's no reason why there should be enough supplyOutput amount in the contract, the contract only ensures minReserve.

Impact

No liquidations can be performed if all tokens are lent out. Example: User A supplies 1k$ WETH, User B supplies 1.5k$ DAI and borrows the ~1k$ WETH (only leaves minReserve). The ETH price drops but user B cannot be liquidated as there's not enough WETH in the pool anymore to pay out the liquidator.

Recommendation

Mint LP supply tokens to msg.sender instead, these are the LP supply tokens that were burnt from the borrower. This way the liquidator basically seizes the borrower's LP tokens.

Handle

cmichel

Vulnerability details

There is no check in UniswapV3Oracle.ethPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

It also seems to use the deprecated API (latestAnswer) and a hardcoded decimals of 1e8?

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the liquidation pricing.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

The LendingPair._safeTransfer function will revert if tokens do not return a boolean because the interface ERC20.transfer function it uses indicates that this function always returns a boolean. Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommendation

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer function that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The total debt and total supply only increase when debt/supply is minted to the user when it should increase by the entire new interest amount on each accrual.

function accrueAccount(address _account) public {
  _distributeReward(_account);
  // accrue only updates cumulativeInterestRate to the newInterest
  // newInterest is not added to total debt or total LP supply!
  accrue();
  // instead total debt / total LP supply is increased here by a much smaller amount, the new interest specific for the updating user
  _accrueAccountInterest(_account);

  if (_account != feeRecipient()) {
    _accrueAccountInterest(feeRecipient());
  }
}

Impact

The borrow rates (see borrowRatePerBlock) are wrong due to the wrong utilisation ratio: The borrow utilisation rate uses LPToken.totalSupply. Assume there's a single lender supplying 100k, another single borrower borrows 70k (ignoring irrelevant details like liquidation and the borrower not putting up collateral for the sake of the argument). After some time debt accrued and the supplier "updates" by calling accrue (but the borrower does not update), this increases the LP total supply to, say, 110k, while total debt is not updated. The utilisation rate and thus the borrow rate is now less than before (from 70/100=70% to 70/110=63%). In reality, it should have increased as the supplier interest is only a fraction of the borrower accumulated debt. From now on less debt than expected accrues until the borrower is updated and total debt is increased. To get correct borrow rates in the current system every borrower and every supplier would need to be updated on every accrual which is infeasible.

Recommended Mitigation Steps

Do it like Compound/Aave, increase total debt and total supply on each accrual by the total new interest (not by the specific user's interest only). This might require a bigger refactor because the LP token is treated as a ("lazy-evaluated") rebasing token and the total supply is indeed the total tokens in circulation LP.totalSupply() and one cannot mint the new interest to all users at once in each accrue. That's why Compound uses an interest-bearing token and tracks total supply separately and Aave uses a real rebasing token that dynamically scales the balance in balanceOf and must not be updated individually for each user.