QA Report

[code423n4]

Buyout can be considered successful with less than 51% of tokens in pool

Impact

It is possible to move the state to success by having only 50.1% of the total circulating tokens in the pool.

The documentation for end() states, "if the current pool has at least 51% of the total circulating supply, then the buyout is considered a success. The remaining fractions in the pool are then burned, allowing the proposer to safely withdraw the underlying assets from the vault."

However, it is possible to call end() and move the state to SUCCESS by having only 50.1% of the total circulating tokens in the pool.

Proof of Concept

For example, assume tokenBalance == 50_100 and IVaultRegistry(registry).totalSupply(_vault) == 100_000. Using these values in the formula below, the result is 501 which equates to 50.1% of the total circulating supply.

// L184
function end(address _vault, bytes32[] calldata _burnProof) external {
    ...
// L210
 uint256 tokenBalance = IERC1155(token).balanceOf(address(this), id);
        // Checks totalSupply of auction pool to determine if buyout is successful or not
        if (
            (tokenBalance * 1000) /
                IVaultRegistry(registry).totalSupply(_vault) > 500
                ){...}
}

Below is a simple fuzz test to validate the percentage formula.

pragma solidity 0.8.9;

import "lib/forge-std/src/Test.sol";

contract Fractional{

    function checkMath (uint tokenBalance, uint totalSupply)public pure returns(uint percent){
        require (tokenBalance > 0);
        require (totalSupply > tokenBalance);
        percent = (tokenBalance * 1000 / totalSupply);
    }
    
}

contract FractionalTest is Test {
    Fractional frac;

    function setUp() public {
        frac = new Fractional();
    }

    function testMath(uint tokenBalance) public  {
        vm.assume(tokenBalance > 0);
        vm.assume(tokenBalance <= 100_000);
        uint percent = frac.checkMath(tokenBalance,100_000);
        assert(percent < 501 || percent > 509);
    }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Consider changing the code in the above if-block to be > 509 or alternatively >= 510 for improved readability and a small increase in gas used.

===========================================================================

Lack of input validation for array length

Impact

In Vault the install function does not validate the passed array lengths which may lead to unintended storage outcomes.

The same issue is present in the BaseVault batch deposit functions: batchDepositERC20, batchDepositERC721 and batchDepositERC1155.

Proof of Concept

In Vault, if a caller passes one bytes4 _selectors and two _plugins addresses to install() then the second _plugins address will not be registered to a methods mapping. This is because the for loop iterates based on the length of the _selectors array.

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider adding require statements in functions to validate user-controlled data input and to ensure that array lengths are equal.

Discrepancy between documentation and code

Impact

The documentation for the Buyout module states that "A fractional owner starts an auction for a vault by depositing any amount of ether and fractional tokens into a pool."

However, in start() the amount of fractional tokens a user must deposit is the entire balance of msg.sender.

link to depositAmount

uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);

Proof of Concept

N/A

Tools Used

Manual Analysis

Recommended Mitigation Steps

Update the documentation to make it clear that a user is required to deposit all of their fractional tokens into a pool.

[HardlyDifficult]

Merging with #320