search

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

0 stars 0 forks source link

Able to call ```withdrawContributions``` with any vaultId may lead to loss of funds #639

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The withdrawContributions function in Migration.sol takes any vault as input. As long as the vault is valid and has an inactive buyout, a user may call withdrawContributions even if the proposal they contributed to is LIVE.

This may lead to users not being able to commit their proposals if the Migrations.sol contract does not contain enough ETH for their commits.

A malicious user may also swap their vault tokens for any other vault tokens in the Migration.sol contract since withdrawContributions sends the tokens of the vault given as input. So if a user joined a proposal with 100 tokens of vault A and calls withdrawContributions with _vault set to vault B, the user will receive 100 vault B tokens.

Proof of Concept

Scenario 1: Malicious user withdraws ETH so that other proposals may not be commited

  1. A user joins a proposal for vault A with 1 ETH, which is then commited and the ETH is sent to Buyout.sol.

  2. Another user joins a proposal for vault B with 1 ETH, which is not yet commited so the ETH stays in Migration.sol.

  3. The first user calls withdrawContributions with the proposalId they called join with and the vault set to vault B (it could be any valid vault).

  4. The withdrawContributions will not revert since the buyout state for vault B is INACTIVE (assuming no one starts a buyout) and the '''migrationInfo[_vault][_proposalId].newVault``` should return a 0 address since the proposalId is valid only for vault A due to the incrementing of the proposalId.

  5. Since the userProposalEth returns 1 ETH, the first user will receive 1 ETH and Migration.sol will lose 1 ETH. Migration.sol now has 0 ETH.

  6. The second user will now not be able to commit their proposal since the Migration.sol contract contains less ETH than the totalEth returned by migrationInfo for that proposalId. Migration.sol does not have enough ETH for the user to call commit.

Scenario 2: Malicious user swaps worthless vault A tokens for valuable vault B tokens

  1. A malicious user joins a proposal for vault A with 1 ETH and 100 vault A tokens.

  2. A proposal for vault B has 100 vault B tokens and is not commited yet.

  3. The proposal for vault A is commited, sending totalEth and totalFractions of the proposal for vault A to the Buyout.sol contract.

  4. The malicious user calls withdrawContributions with _vault set to vault B and using the vault A proposalId.

  5. The withdrawContributions will not revert since the buyout state for vault B is INACTIVE (assuming no one starts a buyout) and the '''migrationInfo[_vault][_proposalId].newVault``` should return a 0 address since the proposalId is valid only for vault A due to the incrementing of the proposalId.

  6. The userProposalFractions for the malicious user will return 100 since they joined the vault A proposal with 100 tokens.

  7. Migration.sol will send the vault B tokens to the malicious user instead of the correct vault A tokens. (Assuming the contract has enough ETH to send to the malicious user as well)

Recommended Mitigation Steps

Since no 2 vaults will have the same proposalId, check that the migrationInfo does not return an empty proposal in withdrawContributions.

KenzoAgada commented 2 years ago

(Duplicate of #326, same as #336)

HardlyDifficult commented 2 years ago

Agree, thanks @KenzoAgada!