Failed proposal can be committed again

[code423n4]

Lines of code

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

Vulnerability details

Impact

Failed proposal can be committed again and eth stolen from migration contract in combination with other vulnerabilities submitted

Proof of Concept

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

commit can be called as long as it has been less than 7 days from the start time. The buyout period is specified as 4 days in the buyout contract. This means that as long as proposal is committed within 3 days of starting, commit can be called again after a failed buyout (4 days) because the current time will still be less than 7 days from the start time.

This can be used in combination with a vulnerability I previously reported. The contract does not account for the actual number of fractions that it receives back from a failed buyout. If it sent 10 fractions and 3 eth to a buyout it may receive back 15 fractions and 2 eth due to trading against the buyout. Because commit can called again on the same proposal, the second time it will try to send the fraction balance of the contract, now 15, and the value of the eth in the proposal, 3 eth. This transaction will either revert due to not having enough eth or it will send 3 eth pulling from eth deposited to other migration proposals.

This could be exploited by creating a vault and immediately migrating it. Once the migration starts the user could sell fractions to themselves and get eth, making sure to keep the number of fractions under 51%, to prevent a successful buyout. After the buyout fails they can then call the commit function again and more eth will be sent. They can then sell fractions to themselves netting more eth than they initially supplied. This could be done repeatedly until all eth has been stolen from the migration contract.

Tools Used

Recommended Mitigation Steps

Change the length of either the migration period or the buyout period to match so that a proposal can't be replayed

[stevennevins]

Duplicate of #251

[HardlyDifficult]

Committing a failed proposal multiple times can steal funds from the migration contract. Agree this is High risk.

Making this submission the primary for talking through the potential vulnerability here.