code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L73-L82

Vulnerability details

Impact

The Vault contract implements the install function which installs plugins by mapping function selectors _selectors to the associated plugin contract addresses _plugins.

However, if the _selector array contains duplicates (e.g. a user accidentally installs plugins and provides duplicate function selectors), the for loop in Vault.install will silently override already assigned function selectors. Depending on the installed plugins, this could cause serious issues due to invoking the wrong (overwritten) function selectors on a vault and therefore calling other than expected plugin contracts.

Proof of Concept

Vault.install

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]; // @audit-info assignment silently overwrites already existing selector <-> plugin mapping
    }
    emit InstallPlugin(_selectors, _plugins);
}

Tools Used

Manual review

Recommended mitigation steps

Consider checking _selectors in Vault.install for function selector duplicates prior to assignment:

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++) {
        require(methods[_selectors[i]] != address(0), "Function selector already in use"); // @audit-info check for already assigned function selector

        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}

mehtaculous commented 2 years ago

Duplicate of #495

HardlyDifficult commented 2 years ago

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 converting this into a QA report for the warden.