Bloated liquidationRate can be set to a Vault

Summary

setVaultConfig() allows for the artificially increased liquidationRate to be set, breaking the subsequent Vault logic. This can happen as uint16 truncation is used, which result is then being controlled, but the initial not truncated value is then set to the configuration structure.

Vulnerability Detail

liquidationRate can be set to a bloated value as its check can be surpassed. For example, vaultConfig.liquidationRate = 756 is now possible to be set, provided that minCollateralRatioBPS = 70. This can be done on purpose as the liquidations mechanics will be silently messed up as the result.

Impact

Liquidation mechanics will be broken with either liquidator receiving an inflated number of shares or liquidations be prohibited as the corresponding calculations will revert.

This can be done either deliberately, for example to use Notional to create non-liquidable Vault, or as a part of operational mistake, that can easily go unnoticed. Either way, the liquidations logic will be violated with either lenders (if liquidations are denied) or borrowers (if liquidators can obtain outsized number of shares) taking a loss.

Code Snippet

setVaultConfig() uses uint16 liquidationRate = uint16(uint256(vaultConfig.liquidationRate - uint256(Constants.PERCENTAGE_DECIMALS)) * uint256(1e2)) for vaultConfig.minCollateralRatioBPS limit check:

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L163-L186

    function setVaultConfig(
        address vaultAddress,
        VaultConfigStorage calldata vaultConfig
    ) internal {
        mapping(address => VaultConfigStorage) storage store = LibStorage.getVaultConfig();
        VaultConfig memory existingVaultConfig = _getVaultConfig(vaultAddress);
        // Cannot change borrow currency once set
        require(vaultConfig.borrowCurrencyId != 0);
        require(existingVaultConfig.borrowCurrencyId == 0 || existingVaultConfig.borrowCurrencyId == vaultConfig.borrowCurrencyId);

        // Liquidation rate must be greater than or equal to 100
        require(Constants.PERCENTAGE_DECIMALS <= vaultConfig.liquidationRate);
        // This must be true or else when deleveraging we could put an account further towards insolvency
        require(vaultConfig.minCollateralRatioBPS < vaultConfig.maxDeleverageCollateralRatioBPS);
        // minCollateralRatioBPS to RATE_PRECISION is minCollateralRatioBPS * BASIS_POINT (1e5)
        // liquidationRate to RATE_PRECISION  is liquidationRate * RATE_PRECISION / PERCENTAGE_DECIMALS (net 1e7)
        //    (liquidationRate - 100) * 1e9 / 1e2 < minCollateralRatioBPS * 1e5
        //    (liquidationRate - 100) * 1e2 < minCollateralRatioBPS
        uint16 liquidationRate = uint16(
            uint256(vaultConfig.liquidationRate - uint256(Constants.PERCENTAGE_DECIMALS)) * uint256(1e2)
        );
        // Ensure that liquidation rate is less than minCollateralRatio so that liquidations are not locked
        // up causing accounts to remain insolvent
        require(liquidationRate < vaultConfig.minCollateralRatioBPS);

However, the initial vaultConfig.liquidationRate is then set to the storage:

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L216

        store[vaultAddress] = vaultConfig;

This way a Vault misconfiguration is possible. As an example, if minCollateralRatioBPS = 70 and vaultConfig.liquidationRate = 756, then uint16 liquidationRate = uint16(uint256(vaultConfig.liquidationRate - uint256(Constants.PERCENTAGE_DECIMALS)) * uint256(1e2)) = ((756 - 100) * 100) % 65535 = 65, so the check passes, but liquidationRate = 756 will go to the storage.

Subsequent liquidation Vault logic will malfunction, with the final outcome depending on the Vault's exact configuration values.

On the one hand, deleverageAccount() vault shares calculation will report the bloated number of vaultSharesToLiquidator:

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L293-L300

        uint256 vaultSharesToLiquidator;
        {
            vaultSharesToLiquidator = vaultAccount.tempCashBalance.toUint()
                .mul(vaultConfig.liquidationRate.toUint())
                .mul(vaultAccount.vaultShares)
                .div(vaultShareValue.toUint())
                .div(uint256(Constants.RATE_PRECISION));
        }

On the other hand calculateDeleverageAmount() can revert, making the liquidations impossible:

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L321-L324

        maxLiquidatorDepositAssetCash = (
            debtOutstanding.mulInRatePrecision(maxCollateralRatioPlusOne).sub(vaultShareValue)
        // Both denominators are in 1e9 precision
        ).divInRatePrecision(maxCollateralRatioPlusOne.sub(vaultConfig.liquidationRate));

Tool used

Manual Review

Recommendation

Consider avoid truncation: