QA Report

[code423n4]

User can have their eth and fraction locked for some time inside Migration contract

Target codebase

Migration.sol#L105-L136

Impact

If a user by mistake join using a _proposalId that isn't created yet, and the vault start new auction while the auction still active. The user funds will be locked inside the Migration contract until auction finish

Recommended Mitigation Steps

To fix this issue is recommended to add a check if proposal is already running:

1. If proposal not exists the startTime = 0
2. If proposal already finished the block.timestamp > proposal.startTime + PROPOSAL_PERIOD

function join(
  address _vault,
  uint256 _proposalId,
  uint256 _amount
) external payable nonReentrant {
  ...
  // Gets the migration proposal for the given ID
  Proposal storage proposal = migrationInfo[_vault][_proposalId];

+ if(proposal.startTime == 0)
+   revert ProposalNotStarted();
+ if(block.timestamp > proposal.startTime + PROPOSAL_PERIOD)
+   revert ProposalOver();

  // Updates ether balances of the proposal and caller
  proposal.totalEth += msg.value;
  ...
}

---

The user may lose all tokens by calling redeem in a vault without started auction

Target codebase

Buyout.sol#L278-L303

Impact

After register a vault in VaultRegistry contract some user with total token supply could call redeem function in Buyout contract without start an auction, losing all tokens.

Proof of Concept

After a user create a vault in VaultRegistry is possible to call the function redeem in Buyout contract without starting the auction:

function redeem(address _vault, bytes32[] calldata _burnProof) external {
+ // The current state is INACTIVE because the user don't create any auction
  (, , State current, , , ) = this.buyoutInfo(_vault);
  State required = State.INACTIVE;
  if (current != required) revert InvalidState(required, current);

  // Initializes vault transaction
  uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
  bytes memory data = abi.encodeCall(
      ISupply.burn,
      (msg.sender, totalSupply)
  );
  // Executes burn of fractional tokens from caller
+ //If the total tokens of the user is the total supply of vault
+ //the burn function is executed in vault
  IVault(payable(_vault)).execute(supply, data, _burnProof);

  // Sets buyout state to successful and proposer to caller
  (buyoutInfo[_vault].state, buyoutInfo[_vault].proposer) = (
      State.SUCCESS,
      msg.sender
  );
  // Emits event for redeem underlying assets from the vault
  emit Redeem(_vault, msg.sender);
}

Recommended Mitigation Steps

To fix this issue is recommended to add another variable to Auction structure, to check if an auction was executed, like this code:

struct Auction {
  // Time of when buyout begins
  uint256 startTime;
  // Address of proposer creating buyout
  address proposer;
  // Enum state of the buyout auction
  State state;
  // Price of fractional tokens
  uint256 fractionPrice;
  // Balance of ether in buyout pool
  uint256 ethBalance;
  // Total supply recorded before a buyout started
  uint256 lastTotalSupply;
+ // Boolean to check is auction already started
+	bool started;
}

---

Should check constructor variables before assign

Target codebase

Minter.sol#L18

Supply.sol#L17

BaseVault.sol#L25

Impact

Is possible to create the contracts without setting the corrects immutable variables.

Recommended Mitigation Steps

Should check for variables in constructor, example:

constructor(
  address _addr
) {
+ if(_addr == address(0)) revert INVALID_PARAM();
  addr = _addr;
}

[HardlyDifficult]

Merging with #229, #228, #226