M-14 Unmitigated

[c4-bot-5]

Lines of code

https://github.com/revert-finance/lend/blob/dfced79e81feba1146bb3936c60931e025f6131e/src/V3Vault.sol#L343

Vulnerability details

C4 Issue

M-14: V3Vault is not ERC-4626 compliant

Issue Details

The original issue was that V3Vault.sol did not implement the ERC-4626 standard properly. More specifically the following functions needed to be refactored in order to comply with the specs defined for ERC-4626 vaults:

• V3Vault::maxDeposit - did not check if the amount exceeds the daily lend limit
• V3Vault::maxMint - did not check if the amount exceeds the daily lend limit
• V3Vault::maxWithdraw - did not check if the amount exceeds the current available balance in the vault
• V3Vault::maxRedeem - did not check if the amount exceeds the current available balance in the vault

The refactoring was needed because the specs explicitly state that the above functions must include any global and user-specific limits

Mitigation

PR-15 attempts to update the V3Vault.sol functions so that it becomes compatible with the ERC-4626 standard. However it fails to fully mitigate the problem, because of incorrect conversion conducted in one of the refactored functions

Conclusion

Not Fully Mitigated

Vulnerability details

The problem lies in the refactoring made inside the maxWithdraw function: https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L345

 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 {
            return _convertToAssets(balance, lendExchangeRateX96, Math.Rounding.Down); // <--- already in assets
        }
    }

If you look closely you will see that inside the else clause balance is converted to assets one more time. This is incorrect since balance already represents the totalAssets in the vault and thus it should not be converted.

This can be confirmed by looking at the _getBalanceAndReserves() function: https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L1112

function _getBalanceAndReserves(uint256 debtExchangeRateX96, uint256 lendExchangeRateX96)
        internal
        view
        returns (uint256 balance, uint256 reserves)
    {
        balance = totalAssets(); // <--- represents assets NOT shares
        .....
    }

Impact

maxWithdraw() artificially inflates the assets and returns incorrect amount

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor maxWithdraw() so that it does not convert assets a seconds time as if they are shares. The final version must look like so:

function maxWithdraw(address owner) external view override returns (uint256) {
        .....
        if (balance > ownerAssetBalance) {
            return ownerAssetBalance;
        } else {
            return balance // <--- not converting again
        }
    }

Assessed type

ERC4626

[jhsagd76]

Nullify this issue, because for the MR-M-14, it will be marked as mitigation confirmed, and the #63 from the same warden(also the unmitigated part of this issue) will be marked as a new issue.

Please note that this will not have any impact on the reward distribution.

confirm this Nullify label

[c4-judge]

jhsagd76 marked the issue as nullified