099.md

Curly Eggplant Bee

Medium

Protocol will Break If `maxVaults' is set Below The Current Number

Summary

A mistake in the contract's design will cause a Denial of Service (DoS) for users as the owner will reduce maxVaults below the current number of whitelisted vaults, preventing new vaults from being added.

Root Cause

In LendingAssetVault.sol :: setMaxVaults(uint8 _newMax), there is no validation to ensure that the new value of maxVaults is not less than the current number of whitelisted vaults. This allows the owner to set maxVaults to a value lower than the existing count of whitelisted vaults, creating an inconsistent state where the contract enforces the limit strictly but does not automatically unwhitelist excess vaults.

function setMaxVaults(uint8 _newMax) external onlyOwner {
    uint8 _oldMax = maxVaults;
    maxVaults = _newMax;
    emit SetMaxVaults(_oldMax, _newMax);
}

Internal Pre-conditions

1. Owner needs to call setMaxVaults() to set maxVaults to be less than the current number of whitelisted vaults.
2. The number of whitelisted vaults must be greater than the new value of maxVaults.

External Pre-conditions

No response

Attack Path

1. Owner calls setMaxVaults(uint8 _newMax) with a value lower than the current number of whitelisted vaults.
2. The contract updates maxVaults but does not unwhitelist any vaults, leaving the number of whitelisted vaults greater than maxVaults.
3. When a user attempts to whitelist a new vault by calling setVaultWhitelist(address _vault, bool _allowed), the contract reverts with the message "M" because the number of whitelisted vaults exceeds maxVaults.

Note: Because this logic error requires the owner to execute the likely hood is medium and impact is high because it causes a DoS

Impact

The users cannot whitelist new vaults, resulting in a Denial of Service (DoS) condition. This prevents legitimate users from interacting with the contract and limits the protocol's functionality. Additionally, the inconsistent state between maxVaults and the actual number of whitelisted vaults introduces operational complexity and potential governance challenges.

PoC

function test_setMaxVaultsBelowCurrentDanger() public {
        // 1. Initial Setup: Allow 10 vaults
        _lendingAssetVault.setMaxVaults(10);

        // 2. Add 10 vaults
        address[] memory vaults = new address[](10);
        for (uint256 i = 0; i < 9; i++) {
            TestERC4626Vault newVault = new TestERC4626Vault(address(_asset));
            address vaultAddr = address(newVault);
            vaults[i] = vaultAddr;
            _lendingAssetVault.setVaultWhitelist(vaultAddr, true);
            console.log("Vaults: ", i);
        }

        // 3. Reduce maxVaults to 6
        _lendingAssetVault.setMaxVaults(6);

        // 4. Verify contract still has 10 whitelisted vaults
        address[] memory whitelisted = _lendingAssetVault.getAllWhitelistedVaults();
        console.log("Number of whitelisted: ", whitelisted.length);
        assertEq(whitelisted.length, 10, "Should retain 10 vaults after max reduction");

        // 5. Test new additions are blocked
        TestERC4626Vault newVault11 = new TestERC4626Vault(address(_asset));
        vm.expectRevert(); // Use correct error encoding
        _lendingAssetVault.setVaultWhitelist(address(newVault11), true);

        // 6. Remove one vault (now 9 left)
        _lendingAssetVault.setVaultWhitelist(vaults[0], false);

        // 7. Verify system remains broken
        vm.expectRevert(); // Still can't add new vaults
        _lendingAssetVault.setVaultWhitelist(address(newVault11), true);

        // 8. Demonstrate DoS: Remove enough vaults to fall below maxVaults
        for (uint256 i = 1; i <= 4; i++) {
            _lendingAssetVault.setVaultWhitelist(vaults[i], false);
        }

        // 9. Verify adding a new vault is now allowed
        _lendingAssetVault.setVaultWhitelist(address(newVault11), true);
    }

Mitigation

Validate to make sure the newMaxVault is greater than the current maxVaults:

function setMaxVaults(uint8 _newMax) external onlyOwner {
+   require(_newMax >= _vaultWhitelistAry.length, "Cannot reduce maxVaults below current count");
    uint8 _oldMax = maxVaults;
    maxVaults = _newMax;
    emit SetMaxVaults(_oldMax, _newMax);
}