Assets can be transferred to zero address on operational mistake

[code423n4]

Lines of code

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

Vulnerability details

It is possible to withdraw all the assets after Buyout before settleVault() was run and newVault created as asset transfer functions do not check the address.

Proof of Concept

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

    /// @notice Migrates an ERC-20 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-20 token
    /// @param _amount Transfer amount
    /// @param _erc20TransferProof Merkle proof for transferring an ERC-20 token
    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(

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

    /// @notice Migrates an ERC-721 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-721 token
    /// @param _tokenId ID of the token
    /// @param _erc721TransferProof Merkle proof for transferring an ERC-721 token
    function migrateVaultERC721(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _tokenId,
        bytes32[] calldata _erc721TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-721 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC721(

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

    /// @notice Migrates an ERC-1155 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-1155 token
    /// @param _id ID of the token
    /// @param _amount amount to be transferred
    /// @param _erc1155TransferProof Merkle proof for transferring an ERC-1155 token
    function migrateVaultERC1155(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _id,
        uint256 _amount,
        bytes32[] calldata _erc1155TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-1155 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC1155(

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

    /// @notice Batch migrates multiple ERC-1155 tokens to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-1155 token
    /// @param _ids IDs of each token type
    /// @param _amounts Transfer amounts per token type
    /// @param _erc1155BatchTransferProof Merkle proof for batch transferring multiple ERC-1155 tokens
    function batchMigrateVaultERC1155(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256[] calldata _ids,
        uint256[] calldata _amounts,
        bytes32[] calldata _erc1155BatchTransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Batch withdraws multiple ERC-1155 tokens from the old vault and transfers to the new vault
        IBuyout(buyout).batchWithdrawERC1155(

Recommended Mitigation Steps

Consider checking for the newVault to be set, i.e. add non-zero check.

[stevennevins]

I do tend to think that this should generally be handled by the token implementation

ie. Solmate ERC20 implementation would be vulnerable to this while OZ's wouldn't

ERC721 from Solmate and OZ have address(0) on the to

And for ERC1155, we're using safeTransferFrom and safeBatchTransferFrom, which both have the address(0) checks for OZ and Solmate. So I think the scope is pretty limited here

[HardlyDifficult]

Duping to https://github.com/code-423n4/2022-07-fractional-findings/issues/459