Fractional v2 contest - async's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L57-L107

Vulnerability details

Impact

The creation of a legitimate auction may be denied by a malicious front-runner indefinitely. This is because a vault may only have one active buyout at a given time.

Proof of Concept

Alice calls start in the Buyout module to initiate an auction for a vault. Along with her fractional tokens, she includes 10 Eth of value for the pool.

Bob, an attacker observing the mempool, can front-run Alice's transaction to start an auction with a price that is far below the vault's market rate.

Bob includes .0001 Eth of value for the pool and submits his transaction with a higher gas fee than Alice to ensure it is included ahead of her transaction in the next block.

Bob has effectively blocked other users from starting good faith auctions for the duration of his auction which is 6 days (2 days of proposal period + 4 days of rejection period).

Bob can repeat this attack indefinitely as long as he is willing to pay a slightly higher gas fee to have his transaction included ahead of a legitimate auction proposers.

Rationale for Medium-severity impact: While the likelihood of this may be low, the impact is high because valid auctions can be prevented from being successfully created indefinitely and will lead to a DoS against the protocol’s functioning. So, with low likelihood and high impact, the severity (according to OWASP) is medium.

Tools Used

Manual analysis

Recommended Mitigation Steps

Mitigate this DoS vector by allowing multiple buyouts at the same time since users will naturally gravitate towards the auction that nets them the highest return.

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.