c4-bot-7 commented 6 months ago

Lines of code

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L345 https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L1384-L1390 https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L1112

Vulnerability details

Lines of code

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L345

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L1384-L1390

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L1112

Vulnerability details

When V3Vault.maxWithdraw() is called, the V3Vault balance is retrieved from _getBalanceAndReserves(). This function will return the total assets owned by the vault. Further along in maxWithdraw(), the Vault will then convert these total assets (aka balance denominated as assets) into assets again via the _convertToAssets() function. This is not needed as balance is already in assets denomination.

Impact

A larger amount of assets will be returned in maxWithdraw() since _convertToAssets() will make balance larger than it is since the exchange rate for shares -> assets is typically greater than 1.

Proof of Concept

The _convertToAssets() function is used when the ownerAssetBalance is greater than or equal to balance. The balance is in assets, as it comes from _getBalanceAndReserves(), which gets the balance from totalAssets(), showing the token's total balance.

function maxWithdraw(address owner) external view override returns (uint256) {
    (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _calculateGlobalInterest();

    uint256 ownerShareBalance = balanceOf(owner);
    uint256 ownerAssetBalance = _convertToAssets(ownerShareBalance, lendExchangeRateX96, Math.Rounding.Down);

    (uint256 balance,) = _getBalanceAndReserves(debtExchangeRateX96, lendExchangeRateX96);
    if (balance > ownerAssetBalance) {
        return ownerAssetBalance;
    } else {
        // AUDIT: balance is already denominated in assets. There is no need to convert to assets again.
->      return _convertToAssets(balance, lendExchangeRateX96, Math.Rounding.Down);
    }
}

function _getBalanceAndReserves(uint256 debtExchangeRateX96, uint256 lendExchangeRateX96)
    internal
    view
    returns (uint256 balance, uint256 reserves)
{
    // AUDIT: totalAssets() will return total asset balance denominated in assets
->  balance = totalAssets();
    uint256 debt = _convertToAssets(debtSharesTotal, debtExchangeRateX96, Math.Rounding.Up);
    uint256 lent = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
    reserves = balance + debt > lent ? balance + debt - lent : 0;
}

Tools Used

Manual review

Recommended Mitigation Steps

Return the balance when the balance can't cover the owner's balance:

function maxWithdraw(address owner) external view override returns (uint256) {
    (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _calculateGlobalInterest();

    uint256 ownerShareBalance = balanceOf(owner);
    uint256 ownerAssetBalance = _convertToAssets(ownerShareBalance, lendExchangeRateX96, Math.Rounding.Down);

    (uint256 balance,) = _getBalanceAndReserves(debtExchangeRateX96, lendExchangeRateX96);
    if (balance > ownerAssetBalance) {
        return ownerAssetBalance;
    } else {
        // AUDIT: balance is returned. No need for further conversion.
->      balance;
    }
}