`methods` collision in Vault

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L70-L82

Vulnerability details

Impact

The owner can install plugins in a Vault using the following function:

function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}

The problem is that there may be multiple identical selectors. In this case, the plugins overwrite each other and only the last one is saved in methods.

4 bytes collision aren't common but neither that rare, and they should be handled appropriately. This situation may happen since the functions are from different contracts.

Proof of Concept

The owner installs two different plugins, both having a function foo(). Only one of them is saved in methods, meaning that the other function can't be called. This may lead to the loss of important functions for the Vault.

Recommended Mitigation Steps

Revert if methods[_selectors[i]] is already different from zero when installing. Replacing a method can still be done by calling uninstall first.

[mehtaculous]

Duplicate of #495

[HardlyDifficult]

By replacing the previous plugin using the same selector, this seems to result in a silent upgrade to the latest plugin. Agree this may lead to unintentional changes, but in that case the owner should be able to install again correcting the problem. Adding a check in this flow to make upgrades more explicit seems like a nice to have. Lowering severity and merging with the warden's QA report #638