Venus Protocol Isolated Pools - BoltzmannBrain's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/WhitePaperInterestRateModel.sol#L17

Vulnerability details

Impact

In WhitePaperInterestRateModel, the code is using 2102400 as its value of blocksPerYear. This is equivalent to a block every 15 seconds (a.k.a. 4 times a minute), since 4 * 60 * 24 * 365 == 2102400. However, Venus protocol is deployed on BSC, which produces a block every 3 seconds, so blocksPerYear should be 5 times larger at 10512000 (which matches the value in BaseJumpRateModelV2). Because of this discrepancy, every market that uses the WhitePaperInterestRateModel will have interest rates 5x larger than they were intended when baseRatePerYear and multiplierPerYear was passed in the constructor. This can lead to unexpected behavior and individuals might accidentally borrow at a much higher rate than they expected.

Proof of Concept

See blocksPerYear.

Tools Used

Manual review.

Recommended Mitigation Steps

Change blocksPerYear to 10512000 in WhitePaperInterestRateModel. Also keep this in mind for the future - if BSC produces blocks at a different rate, then the interest rate models should be updated.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Comptroller.sol#L450

Vulnerability details

Impact

In the liquidateAccount control flow, the user must specify the repayAmount for each element of the orders array. This value is passed directly to the VToken function forceLiquidateBorrow, which eventually directly passes it to preLiquidateHook. Since skipLiquidityCheck is hardcoded to true in this control flow, the preLiquidateHook will revert if repayAmount > borrowBalance. On the other hand, if repayAmount < borrowBalance, then the borrowBalance == 0 check at the end of liquidateAccount will cause a revert.

So, the liquidateAccount function must be called with the exact correct repayAmount. Since this exact correct amount changes every 3s as the account accumulates interest each block, it will be very difficult for a user to liquidate an account properly. Moreover, a motivated attacker could frontrun and slightly modify their underwater positions so that any attempt to liquidate them will revert d/t an incorrect repayAmount. The attacker benefits from not being liquidated, so there is an incentive to do this.

Proof of Concept

This check causes repayAmount > borrowBalance to revert. This check causes repayAmount < borrowBalance to revert.

Also, you can verify this by changing this test case to be 1 wei larger or 1 wei smaller to confirm that both will revert (with either TooMuchRepay or Nonzero borrow balance after liquidation).

Tools Used

Manual review.

Recommended Mitigation Steps

In the liquidateAccount control flow, the liquidator should be allowed to safely "overshoot" the repay amount, similar to the check that happens here. To achieve this logic, I would recommend adding something like this to the start of liquidateAccount:

uint256 accountBorrowsPrev = _borrowBalanceStored(borrower);
repayAmount = repayAmount > accountBorrowsPrev ? accountBorrowsPrev : repayAmount;

Assessed type

DoS