Fractional v2 contest - ak1's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L433-L482

Vulnerability details

Impact

The caller can claim more than his/her shares of fractional tokens. Since the caller can claim more than his/her tokens, I would put this under high.

Proof of Concept

function migrateFractions(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not successful
    (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    State required = State.SUCCESS;
    if (current != required) revert IBuyout.InvalidState(required, current);
    // Reverts if proposer of buyout is not this contract
    if (proposer != address(this)) revert NotProposalBuyout();


    // Gets the last total supply of fractions for the vault
    (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    // Calculates the total ether amount of a successful proposal
    uint256 totalInEth = _calculateTotal(
        1 ether,
        lastTotalSupply,
        migrationInfo[_vault][_proposalId].totalEth,
        migrationInfo[_vault][_proposalId].totalFractions
    );
    // Calculates balance of caller based on ether contribution
    uint256 balanceContributedInEth = _calculateContribution(
        totalInEth,
        lastTotalSupply,
        userProposalEth[_proposalId][msg.sender],
        userProposalFractions[_proposalId][msg.sender]
    );


    // Gets the token and fraction ID of the new vault
    address newVault = migrationInfo[_vault][_proposalId].newVault;
    (address token, uint256 newFractionId) = IVaultRegistry(registry)
        .vaultToToken(newVault);
    // Calculates share amount of fractions for the new vault based on the new total supply
    uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
    uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
        totalInEth;


    // Transfers fractional tokens to caller based on share amount
    IFERC1155(token).safeTransferFrom(
        address(this),
        msg.sender,
        newFractionId,
        shareAmount,
        ""
    );
}

The caller can call this function even after claiming his/her tokens. This allows the caller to claim more tokens. There are no stop checks to avoid this.

Tools Used

Manual code review

Recommended Mitigation Steps

There are more than one solution to avoid this. I have listed few of them below.

1. By adding new enum variable that can be set true once the caller claim tokens, This enum's state can be checked when caller calls this function. 2.The caller balance can be reduced from proposal balance value once the claim is completed.

The current implementation allows for vault creation without any modules. This can be misused by the bad actors for creating vault without any modules. This could be impact the credibility of the protocol.

Suggestion/solution : We would enforce the front end to display vaults with a known combination of modules that we know about. A vault with no modules wouldn’t be displayed in that case.

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L34-L51