Inefficient Removal of Vaults in VaultRegistry.sol

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/VaultRegistry.sol#L80-L93

Vulnerability details

Impact

_removeVaultFromList function has an inefficient way of removing vaults from the 'vaultList' array. The function iterates over the 'vaultList' array until it finds a match, then it moves the last element to the position of the loop iterator and pops the last element from the array.

This could lead to high gas usage when removing vaults, particularly when the 'vaultList' array grows large.

Furthermore, the operation is not entirely safe as it can potentially reorder the vaults in the 'vaultList' array, which could lead to unexpected behavior. This function violates the "Principle of Least Astonishment," which states users should not be surprised by the behavior of a system.

Proof of Concept

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/VaultRegistry.sol#L80-L93

function _removeVaultFromList(ICDPVault vault) private {
    uint256 vaultLen = vaultList.length;
    for (uint256 i = 0; i < vaultLen; ) {
        if (vaultList[i] == vault) {
            vaultList[i] = vaultList[vaultLen - 1];
            vaultList.pop();
            break;
        }

        unchecked {
            ++i;
        }
    }
}


## Tools Used

Manual Review

## Recommended Mitigation Steps

Implementing data structures like mapping and sets where removal is an O(1) operation might be a better strategy. Consider installing OpenZeppelin's EnumerableSet library or IterTools for this purpose.

In addition, a 'vaultExists' boolean variable can be added to the Vault to check if it's registered or removed. This variable can be updated every time a Vault is added or removed to prevent unexpected behaviors and keep the order of the vaults in 'vaultList' consistent. Always notify users whenever there are significant changes to the state of Vaults via events.



## Assessed type

Other