FactoryDAO contest - hickuphh3's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L48 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L53

Vulnerability details

Details & Impact

If msg.value exceeds gate.ethCost, the difference remains permanently stuck in the price gate because the call to the beneficiary is only gate.ethCost.

Recommended Mitigation Steps

Just like the speedBumpGate, the call to the beneficiary should be msg.value instead of gate.ethCost.

(bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}("");

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L224-L234

Vulnerability details

Details & Impact

A reward pool that is created with a malicious reward token (Eg. one that intentionally always returns false on only transfer calls) will brick user withdrawals.

Proof of Concept

• Attacker creates a malicious reward token (eg. MAL) that works normally for .transferFrom() calls such that fundPool() will not revert, but always returns false for .transfer()

• Create a reward pool with USDC as the deposit token and MAL as the sole reward token

• A user deposits into the pool, and after some time, attempts to withdraw his USDC

• Withdrawal reverts because

  IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);

  will be false

Recommended Mitigation Steps

Create an emergencyWithdraw() function that allows redemption of receipts for only deposited funds (forego rewards).

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L193 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L198

Vulnerability details

Details & Impact

Certain ERC20 tokens make modifications to their ERC20's transfer or balanceOf functions. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

These deflationary tokens are incompatible as a reward token, and more importantly, a deposit token for basic staking because the amount recorded (receipt.amountDepositedWei = amount;) is less than the actual amount received. Attempts to redeem the receipt may fail because the contract could hold insufficient tokens.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring calls.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L259 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L264

Vulnerability details

Details & Impact

It is possible for coinsPerSecond to be zero. In these cases, the startTime calculation

uint startTime = block.timestamp + vestingTime - (totalCoins / coinsPerSecond);

will revert from division by zero, preventing initialization, and by extension, withdrawals of vested tokens.

Proof of Concept

We assume vesting time chosen is the maximum (tree.maxEndTime) so that totalCoins = maxTotalPayments. These examples showcase some possibilities for which the calculated coinsPerSecond can be zero.

Example 1: High upfront percentage

• pctUpFront = 99 (99% up front)
• totalCoins = 10_000e6 (10k USDC)
• vestingTime = 1 year

uint coinsPerSecond = (totalCoins * (uint(100) - tree.pctUpFront)) / (vestingTime * 100);
// 10_000e6 * (100 - 99) / (365 * 86400 * 100)
// = 0

Example 2: Small reward amount / token decimals

• pctUpFront = 0
• totalCoins = 100_000e2 (100k EURS)
• vestingTime = 180 days

uint coinsPerSecond = (totalCoins * (uint(100) - tree.pctUpFront)) / (vestingTime * 100);
// 100_000e2 * 100 / (180 * 86400 * 100)
// = 0

Recommended Mitigation Steps

Scale up coinsPerSecond by PRECISION, then scale down when executing withdrawals. While it isn’t foolproof, the possibility of coinsPerSecond being zero is reduced significantly.

// L264
uint coinsPerSecond = (totalCoins * (uint(100) - tree.pctUpFront)) * PRECISION / (vestingTime * 100);

// L184
currentWithdrawal = (block.timestamp - tranche.lastWithdrawalTime) * tranche.coinsPerSecond / PRECISION;

Low Severity Findings

L01: PermissionlessBasicPoolFactory: globalTaxPerCapita bound is unchecked in constructor

Line References

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L75-L78

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L316

Description

The setGlobalTax() has an exclusive upper bound of 1000 on the globalTaxPerCapita value to be set, but this upper bound is not enforced in the constructor.

Recommended Mitigation Steps

constructor(address _globalBeneficiary, uint _globalTaxPerCapita) {
  globalBeneficiary = _globalBeneficiary;
  require(_globalTaxPerCapita < 1000, 'Tax too high');
  globalTaxPerCapita = _globalTaxPerCapita;
}

L02: PermissionlessBasicPoolFactory: Return early in getRewards() if receipt has been redeemed

Line References

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L156-L173

Description

Consider returning zero amounts if the receipt has already been redeemed.

Recommended Mitigation Steps

// after L167
if (receipt.timeWithdrawn != 0) return rewardsLocal;

L03: VoterID: Incomplete contract description

Line References

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L10

Recommended Mitigation Steps

Complete the missing description of the contract.

Non-Critical Findings

NC01: Inconsistent syntax in MerkleEligibility.addGate()

Line References

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L47

Description

The other gates use the prefix operator ++numGates, should do the same here for consistency.