code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L334-L350 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Buyout.sol#L343-L367

Vulnerability details

Impact

HIGH - Assets can be lost directly After proposal and proposed buyout is successful, anyone can transfer ERC20 asset in the vault to the zero address and the asset will be lost.

Proof of Concept

proof of concept: testAnyoneCanThrowERC20_poc

The proof of concept demonstrates a scenario, where alice is sending ERC20 asset from bob's vault to zero address, so the asset is lost.

set up vault with ERC20
bob proposes (proposalId 1)
bob commits to kickoff the buyout process
after the buyout succeeds bob calls end on buyoutModule
now alice can call migrateVaultERC20 with proposalId 2 and send ERC20 to the zero address

// modules/Migration.sol
// in the line 341, zero address for newVault was not checked

334     function migrateVaultERC20(
335         address _vault,
336         uint256 _proposalId,
337         address _token,
338         uint256 _amount,
339         bytes32[] calldata _erc20TransferProof
340     ) external {
341         address newVault = migrationInfo[_vault][_proposalId].newVault;
342         // Withdraws an ERC-20 token from the old vault and transfers to the new vault
343         IBuyout(buyout).withdrawERC20(
344             _vault,
345             _token,
346             newVault,
347             _amount,
348             _erc20TransferProof
349         );
350     }

The migrateVaultERC20 does not check whether the newVault is zero address or not. It just sends ERC20 to the newVault. The buyout module does not check whether to address is zero either: (code). The target Transfer does not check it, either. Some ERC20 tokens will refuse to send to zero address, but some will just send it. For example, the MockERC20 based on solmate's ERC20 does not check for zero address. And The standard does not require to throw upon sending to zero address. Migrate functions for ERC721 and ERC1155 are safe for this issue because they must throw upon sending to zero address. Also The Target Transfer for ERC1155 uses safeTransferFrom.

Tools Used

foundry

Recommended Mitigation Steps

Add zero address check in the migrateVaultERC20 for the address newVault. Also add to check whether the propose was successful. The lack of check can be exploited in a different way combined with other issues. (see the issue Migration Module: The assets can be taken by a failed proposal)

stevennevins commented 2 years ago

Duplicate of #649

HardlyDifficult commented 2 years ago

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

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