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

0 stars 0 forks source link

QA Report #638

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

(L1) Starting/joining/leaving a migration proposal shouldn't depend on Buyout state

In Migration.sol propose function (and similarly in join and leave):

// Reverts if buyout state is not inactive
(, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
State required = State.INACTIVE;
if (current != required) revert IBuyout.InvalidState(required, current);

I don't see the need to check the Buyout state at these steps. This check only slows a possible proposal initiation.

(L2) call instead of transfer

At the end of Migration.sol#leave and #withdrawContribution, ether is sent back to the caller.

// Withdraws ether from contract back to caller
payable(msg.sender).transfer(ethAmount);

As usual, if msg.sender is a contract that performs some operation on fallback, the transfer may out-of-gas and revert, leading to the impossibility to withdraw that funds. Consider using the low-level call instead. No re-entrancy issues, since this line is at the end of the function.

(L3) Migration proposals lasts indefinetely

According to the documentation, "For 7 days, users can contribute their fractions / ether to signal support" to a proposal (https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L23). However an user can call join for a proposal regardless of the time.

Consider adding a requirement in Migration.sol#join like require(block.timestamp <= proposal.startTime + PROPOSAL_PERIOD).

(N1) Emit an event on proposal creation

Something like a proposal creation needs to be easily caught off-chain. Consider adding a specific event at the end of Migration.sol#propose.

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L98

(N2) Events SellFractions and BuyFractions may need a vault parameter

Right now the events SellFractions and BuyFractions don't track for which vault the user selled their fractions. Such a parameter should be essential to track the buyout of a vault.

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L143 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L178

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-07-fractional-findings/issues/606, https://github.com/code-423n4/2022-07-fractional-findings/issues/601