Fractional v2 contest - ayeslick's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L58 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L77 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L98

Vulnerability details

Impact

When a call is made to a given token within BaseVault the token contract sees BaseVaults` address not the callers' address. As a result, all of the checks pass, and the caller is able to drain the contract.

Proof of Concept

A caller can pass an address they control into the _to parameter while passing in BaseVault's address to the _from parameter and setting the _amounts and _tokens arrays equal to the types and amounts of tokens within BaseVault. Then the caller can transfer all of the tokens to themselves.

Recommended Mitigation Steps

Set the _from parameter to msg.sender Set the _to parameter to address(this)

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L76 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L87 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L94 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L102

Vulnerability details

Impact

When DELEGATECALL is called the targets' code is executed within Vault's storage/context. The targets' code is not limited to the functions within the Vault which means the targets' code may manipulate the Vault's storage. State variables owner, merkleRoot, nonce, and the methods mapping can be changed. MIN_GAS_RESERVE is a constant so it cannot be changed after the contract is deployed. The only variable that is checked to make sure it wasn't changed is the owner variable. This leaves the rest susceptible to manipulation where the values can be changed without proper permission.

The target contract could also call selfdestruct() which would destroy the vault.

Proof of Concept

For example the install function restricts access to its execution by checking to make sure msg.sender is the owner of the contract. This works under normal circumstances. However when a DELEGATECALL is executed the targets' contract has complete access to the context/storage of the Vault contract and, the targets' contract, isn't bound by the Vault's contract access-controlled functions. Here is the install 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);
    }

Here is the targets code to install plugins

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);
    }

By commenting out the check the targets' code is free to install any plugin when a DELEGATECALL is executed.

Recommended Mitigation Steps

Check the other variables the same way the owner variable is checked.

Report

Underflow in log2ceil_naive()

Impact

Its possible to underflow log2ceil_naive() by passing in 0. https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L190

Proof of Concept

If caller passes 0 into log2ceil_naive() it will get overflow, 115792089237316195423570985008687907853269984665640564039457584007913129639935.

Recommended Mitigation Steps

log2ceil_naive should be internal or private to prevent anyone from calling it