Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #638

Open
code423n4 opened this issue Jul 14, 2022 · 1 comment
Open

QA Report #638

code423n4 opened this issue Jul 14, 2022 · 1 comment
Labels
bug Warden finding QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

(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

@code423n4 code423n4 added bug Warden finding QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 14, 2022
code423n4 added a commit that referenced this issue Jul 14, 2022
@mehtaculous mehtaculous added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Jul 25, 2022
@HardlyDifficult
Copy link
Collaborator

HardlyDifficult commented Aug 3, 2022

Merging with #606, #601

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Warden finding QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants