search

code-423n4 / 2022-07-fractional-findings

0 stars 0 forks source link

Migration's withdrawContribution ignores the exchange between fractional tokens and ETH happened during Buyout attempt #634

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

withdrawContribution() aims to return the funds to Migration participants. However, it uses initial userProposalFractions[_proposalId][msg.sender] and userProposalEth[_proposalId][msg.sender] records for withdrawal accounting. Real funds structure is different after Buyout as it allows for exchange between tokens and ETH. This way the system becomes insolvent for some users, having excess funds that will be frozen.

Setting the severity to be high as that's principal fund freeze impact.

Proof of Concept

Currently withdrawContribution() does not account for the fact that fractional tokens and ETH composition of the contract balance was changed during the Buyout attempt:

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

    /// @notice Retrieves ether and fractions deposited from an unsuccessful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the failed proposal
    function withdrawContribution(address _vault, uint256 _proposalId)
        external
    {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        if (id == 0) revert NotVault(_vault);
        // Reverts if caller has no fractional balance to withdraw
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

        // Temporarily store user's fractions for the transfer
        uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
        // Updates fractional balance of caller
        userProposalFractions[_proposalId][msg.sender] = 0;
        // Withdraws fractional tokens from contract back to caller
        IFERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            userFractions,
            ""
        );

        // Temporarily store user's eth for the transfer
        uint256 userEth = userProposalEth[_proposalId][msg.sender];
        // Udpates ether balance of caller
        userProposalEth[_proposalId][msg.sender] = 0;
        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(userEth);
    }

Recommended Mitigation Steps

Consider introduction of new balance calculations: each fractional tokens bought out during the Buyout attempt should be replaced with ETH funds received and vice versa.

stevennevins commented 2 years ago

Duplicate of #375