Fractional v2 contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

After passing in the wrong ID with eth, if a new proposal is submitted with the new ID and it succeeds, the transfer eth for the wrong ID will be lost forever

Proof of Concept

join() no detection of whether the proposal id has been created

    function join(
        address _vault,
        uint256 _proposalId,
        uint256 _amount
    ) external payable nonReentrant {
        ....

        Proposal storage proposal = migrationInfo[_vault][_proposalId];

        proposal.totalEth += msg.value;                    //************no detection of whether the proposal id has been created******//
        userProposalEth[_proposalId][msg.sender] += msg.value;
       ....

If there is a successful new proposal for the same vault, the wrong proposal above will be lost eth forever

    function leave(address _vault, uint256 _proposalId) external {
        .....
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        State required = State.INACTIVE;
        if (current != required) revert IBuyout.InvalidState(required, current);   //****If there is a successful new proposal for the same vault, the wrong proposal above will be lost eth forever   *****/
        ...

Tools Used

Recommended Mitigation Steps

join detection of whether the proposal id has been created and valid

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L152-L161 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L211 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L223-L226

Vulnerability details

Impact

Detailed description of the impact of this finding. Migration.sol does not prevent multiple proposals for one vault

submit a malicious proposal to steal vault from a normal proposal

Proof of Concept

Migration.sol does not prevent multiple proposals for one vault

submit a malicious proposal such as proposal.id: 100 for valut[a], set malicious plugins and set a very low _targetPrice price and then submit the proposal, although the buyout will fail,but proposal[100].isCommited = true

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

       if (currentPrice > proposal.targetPrice) {
            // Sets token approval to the buyout contract
            IFERC1155(token).setApprovalFor(address(buyout), id, true);
            // Starts the buyout process
            IBuyout(buyout).start{value: proposal.totalEth}(_vault);
            proposal.isCommited = true;
            started = true;
        }

Monitor if there is a new proposal for vault_a (multiple proposals can exist at the same time for same valut), monitor for a new proposal, and if the corresponding Buyout exceeds 51%, immediately call Buyout.end to make vault_a successful, and call Migration.settleVault to pass in vault_a and the old malicious proposal.id: 100 Then call Migration.migrateVaultERC20() to pass in vault_a and the old malicious proposal proposal.id:100, and transfer the ERC20 through the malicious plugins

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

    function settleVault(address _vault, uint256 _proposalId) external {
        // Reverts if the migration was not proposed
        Proposal storage proposal = migrationInfo[_vault][_proposalId];
        if (!(proposal.isCommited)) revert NotProposed();
        // Reverts if the migration was unsuccessful
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (current != State.SUCCESS) revert UnsuccessfulMigration();

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

    function migrateVaultERC20(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _amount,
        bytes32[] calldata _erc20TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-20 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC20(
            _vault,
            _token,
            newVault,
            _amount,
            _erc20TransferProof
        );
    }

Tools Used

Recommended Mitigation Steps

add the proposal maximum expiration time, if the proposal is add if there exists other proposal and has expired to clear other proposals