Back to reassor's contests

FactoryDAO contest - reassor's results

The DAO that builds DAOs.

General Information

Platform: Code4rena

Start Date: 04/05/2022

Pot Size: $50,000 DAI

Total HM: 24

Participants: 71

Period: 5 days

Judge: Justin Goro

Total Solo HM: 14

Id: 119

League: ETH

FactoryDAO

Findings Distribution

Researcher Performance

Rank: 12/71

Findings: 6

Award: $680.69

🌟 Selected for report: 1

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryFactoryDAO

Findings Information

🌟 Selected for report: cccz

Also found by: 0x52, 0xYamiDancho, GimelSec, IllIllI, PPrieditis, WatchPug, csanuragjain, danb, gzeon, hickuphh3, horsefacts, hyh, kenzo, leastwood, reassor, unforgiven

Awards

63.9296 DAI - $63.93

Labels

bug
duplicate
3 (High Risk)

External Links

Link to GitHub issue

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

Impact

Contract FixedPricePassThruGate is a pass thru gate that is passing funds to the gate's beneficiary. Function passThruGate requires to send ether that is equal or more than gate.ethCost. In the case of receiving more ether than gate.ethCost, passThruGate passes to the beneficiary only amount of gate.ethCost:

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

Since there is no way to withdraw ether it ends up with ether being locked forever in the contract and effectively loss of funds for the user(s).

Proof of Concept

FixedPricePassThruGate.sol:

Tools Used

Manual Review / VSCode

It is recommended to either pass msg.value to beneficiary:

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

or make sure that exact amount of ether is sent:

require(msg.value == gate.ethCost, 'Please send exact amount of ETH');

#0 - illuzen

2022-05-11T09:33:26Z

Duplicate #49

#1 - gititGoro

2022-06-14T02:52:12Z

Upgraded severity: lost user funds.

#2 - gititGoro

2022-06-14T02:53:23Z

Duplicate of #48

Findings Information

🌟 Selected for report: MaratCerby

Also found by: 0x1337, 0x52, 0xYamiDancho, AuditsAreUS, CertoraInc, Dravee, GimelSec, IllIllI, PPrieditis, Ruhum, TrungOre, VAD37, berndartmueller, cccz, csanuragjain, defsec, hickuphh3, horsefacts, hyh, jayjonah8, kenzo, leastwood, mtz, p4st13r4, reassor, throttle, wuwe1, ych18

Awards

3.1753 DAI - $3.18

Labels

bug
duplicate
2 (Med Risk)

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L144-L146 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L194-L198 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L73-L77 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L112-L121 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L85-L89

Vulnerability details

Impact

FactoryDAO is providing multiple tools for DAOs to launch tokens and manage their economies. Multiple contracts that are handling tokens are not supporting ERC20 tokens with fee. This leads to incorrect accounting for depositing tokens for all ERC20 tokens with fee and potentially the one that can such a fee enable. One of the popular tokens that implements such a fee (but currently is set to 0) is USDT.

Example for PermissionlessBasicPoolFactory.deposit:

    (..)
    receipt.amountDepositedWei = amount;
    receipt.timeDeposited = block.timestamp;
    receipt.owner = msg.sender;

    bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
    (..)
  1. User deposits 1000 tokens
  2. Contract saves that user deposited 1000 tokens
  3. transferFrom transferred 999 tokens to the contract and took 0.1% as a fee
  4. Contract thinks that 1000 tokens received, while in reality 999 was received

The severity was increased to medium since the FactoryDAO protocol is expected to be used by DAOs and managing the variety of ERC20 tokens.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

Tools Used

Manual Review / VSCode

It is recommended to add support for ERC20 tokens with built-in fees. Example of the implementation:

(..)
uint256 _beforeBalance = IERC20(pool.depositToken).balanceOf(address(this));
bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
require(success, 'Token transfer failed');
uint256 _afterBalance = IERC20(pool.depositToken).balanceOf(address(this));

receipt.amountDepositedWei = _afterBalance - _beforeBalance
(..)

#0 - illuzen

2022-05-10T07:00:06Z

Duplicate, but this mitigation is better.

Findings Information

🌟 Selected for report: reassor

Also found by: IllIllI, VAD37, hyh, kenzo, leastwood, rajatbeladiya, ych18

Labels

bug
2 (Med Risk)
sponsor confirmed

Awards

105.1961 DAI - $105.20

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Impact

Contract PermissionlessBasicPoolFactory calculates rewards by using hardcoded value of decimals 18 (1e18) for ERC20 tokens. This leads to wrong rewards calculations and effectively loss of funds for all pools that will be using ERC20 tokens with different decimals than 18. Example of such a token is USDC that has 6 decimals only.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add support for different number of decimals than 18 by dynamically checking decimals() for the tokens that are part of the rewards calculations. Alternatively if such a support is not needed, new require statements should be added to addPool that will be checking that the number of decimals for all ERC20 tokens is 18.

#0 - illuzen

2022-05-10T07:00:24Z

Valid

Findings Information

🌟 Selected for report: AuditsAreUS

Also found by: leastwood, pedroais, reassor

Labels

bug
duplicate
2 (Med Risk)
sponsor confirmed

Awards

320.671 DAI - $320.67

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L110 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L314-L318

Vulnerability details

Impact

Contract PermissionlessBasicPoolFactory allows creating new permissionless pools. One of the parameters of the new pool is pool.taxPerCapita that is set to the value of globalTaxPerCapita and represents tax on that specific pool. Account globalBeneficiary is able to change globalTaxPerCapita at any time via setGlobalTax and he can set it up as high as 99%. He is able to run front-running attack that will monitor mempool and once the creation of the new pool with significant amount of rewards gets detected he will change the globalTaxPerCapita to 99%.

Exploit Scenario:

  1. User triggers addPool and transfers 1000 WETH tokens (global tax is set to 0.1%)
  2. Attacker globalBeneficiary monitors mempool and launches front-running attack which increases globalTaxPerCapita to 99% via setGlobalTax function
  3. setGlobalTax is being included in the block before user's addPool
  4. User ends up with creating pool with tax set 99% and significant amount of tokens locked as reward that he cannot withdraw
  5. Attacker/other users starts depositing tokens to the pool to get rewards
  6. At the end of pool attacker collects 99% of the pool rewards as a tax

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add timelock for changing globalTaxPerCapita via setGlobalTax.

#0 - illuzen

2022-05-10T06:05:37Z

Valid, but probably tax should be passed into function and checked to match, instead of timelock

#1 - illuzen

2022-05-10T07:18:51Z

Closing since https://github.com/code-423n4/2022-05-factorydao-findings/issues/56 has a better mitigation

#2 - illuzen

2022-06-03T02:40:15Z

https://github.com/code-423n4/2022-05-factorydao/pull/2

#3 - gititGoro

2022-06-12T04:10:58Z

Duplicate of https://github.com/code-423n4/2022-05-factorydao-findings/issues/56

Findings Information

🌟 Selected for report: horsefacts

Also found by: 0x1f8b, 0xYamiDancho, 0xf15ers, 0xkatana, ACai, AlleyCat, Bruhhh, Dravee, Funen, GimelSec, Hawkeye, IllIllI, MaratCerby, PPrieditis, Picodes, Ruhum, TerrierLover, VAD37, berndartmueller, csanuragjain, defsec, delfin454000, eccentricexit, ellahi, fatherOfBlocks, gzeon, hansfriese, hickuphh3, hyh, ilan, joestakey, juicy, kebabsec, oyc_109, rajatbeladiya, reassor, rfa, robee, samruna, simon135, sorrynotsorry, throttle

Awards

127.9429 DAI - $127.94

Labels

bug
QA (Quality Assurance)

External Links

Link to GitHub issue

1. Missing zero address checks

Risk

Low

Impact

FactoryDAO's contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality

Proof of Concept

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

MerkleIdentity.sol:

MerkleEligibility.sol:

FixedPricePassThruGate.sol:

VoterID.sol:

PermissionlessBasicPoolFactory.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

2. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

MerkleIdentity.sol:

MerkleEligibility.sol:

FixedPricePassThruGate.sol:

SpeedBumpPriceGate.sol:

PermissionlessBasicPoolFactory.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing events to listed functions.

3. Use SafeERC20 safeTransfer

Risk

Low

Impact

The IERC20.transfer() function returns a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Furthermore, tokens that do not correctly implement the EIP20 standard, like USDT which does not return a success boolean, will revert.

Proof of Concept

MerkleVesting.sol:

Tools Used

Manual Review / VSCode

It is recommended to use OpenZeppelin's SafeERC20 with the safeTransfer function that handles the return value check as well as non-standard-compliant tokens.

4. Not following checks-effects-interactions pattern

Risk

Low

Impact

Function PermissionlessBasicPoolFactory.fundPool does not follow checks-effects-interactions pattern that might lead to reentrancy attacks.

    success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
    // bookkeeping to make sure pools don't share tokens
    pool.rewardFunding[i] += amount;

Proof of Concept

PermissionlessBasicPoolFactory.sol

Tools Used

Manual Review / VSCode

It is recommended to first set the effects and then perform interactions such as external calls.

5. Owner - critical address change

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

MerkleIdentity.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for passing ownership for HolyPaladinToken.sol and PaladinRewardReserve.sol contracts.

6. Missing validation

Risk

Low

Impact

Missing check for _globalTaxPerCapita that should not exceeds 1000.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add check for _globalTaxPerCapita to make sure that the value cannot be bigger than 1000.

7. VoterID tokenURI does not revert for non existent tokens

Risk

Non-Critical

Impact

Function VoterID.tokenURI that implements ERC721 standard should revert in case of non existent tokens.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to revert for non existent tokens.

8. Contracts use different compiler versions

Risk

Non-Critical

Impact

Using different compiler versions across contracts of the same project might lead to confusion and accidental errors.

Proof of Concept

Examples:

Tools Used

Manual Review / VS Code

Consider using a single compiler version for compiling both contracts, for example 0.8.12.

#0 - illuzen

2022-05-12T08:49:30Z

all duplicates

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0xNazgul, 0xYamiDancho, 0xf15ers, 0xkatana, ACai, CertoraInc, Dravee, Funen, GimelSec, Hawkeye, PPrieditis, Picodes, Ruhum, TerrierLover, Tomio, VAD37, Waze, csanuragjain, defsec, delfin454000, eccentricexit, ellahi, fatherOfBlocks, gzeon, hansfriese, horsefacts, ilan, joestakey, juicy, minhquanym, oyc_109, rajatbeladiya, reassor, rfa, robee, samruna, simon135, z3s

Awards

59.7678 DAI - $59.77

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

1. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

PermissionlessBasicPoolFactory

MerkleLib.sol:

Tools Used

Manual Review / VSCode

It is recommended to cache the array length outside of for loop.

2. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

MerkleIdentity.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

It is recommended to decrease revert messages to maximum 32 bytes. Alternatively custom error types should be used.

3. Use custom errors instead of revert strings to save gas

Impact

Usage of Custom Errors reduces the gas cost.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

    require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');
    require(success, 'Token deposits failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(receipt.id == receiptId, 'Uninitialized receipt');
    require(pool.id == poolId, 'Uninitialized pool');
    require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
    require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
    require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(receipt.id == receiptId, 'Can only withdraw real receipts');
    require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
    require(receipt.timeWithdrawn == 0, 'Can only withdraw once per receipt');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');
    require(block.timestamp > pool.endTime, 'Contract must reach maturity');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(success, 'Token transfer failed');
    require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');
    require(newTaxPerCapita < 1000, 'Tax too high');

MerkleDropFactory.sol:

    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(treeIndex <= numTrees, "Provided merkle index doesn't exist");
    require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token.");
    require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
    require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");

MerkleVesting.sol:

    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(!initialized[destination][treeIndex], "Already initialized");
    require(tree.rootHash.verifyProof(leaf, proof), "The proof could not be verified.");
    require(initialized[destination][treeIndex], "You must initialize your account first.");
    require(block.timestamp > tranche.lockPeriodEndTime, 'Must wait until after lock period');
    require(tranche.currentCoins >  0, 'No coins left to withdraw');

MerkleResistor.sol:

    require(pctUpFront < 100, 'pctUpFront >= 100');
    require(minEndTime < maxEndTime, 'minEndTime must be less than maxEndTime');
    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(msg.sender == destination, 'Can only initialize your own tranche');
    require(!initialized[destination][treeIndex], "Already initialized");
    require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
    require(valid, 'Invalid vesting schedule');
    require(initialized[destination][treeIndex], "You must initialize your account first.");
    require(tranche.currentCoins >  0, 'No coins left to withdraw');
    require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');

FixedPricePassThruGate.sol:

    require(msg.value >= gate.ethCost, 'Please send more ETH');
    require(sent, 'ETH transfer failed')

SpeedBumpPriceGate.sol:

    require(msg.value >= price, 'Please send more ETH');
    require(sent, 'ETH transfer failed');

MerkleEligibility.sol:

    require(msg.sender == gateMaster, "Only gatemaster may call this.");
    require(isEligible(index, recipient, proof), "Address is not eligible");

MerkleIdentity.sol:

    require (msg.sender == management, 'Only management may call this');
    require(msg.sender == treeAdder, 'Only treeAdder can add trees');
    require(merkleIndex <= numTrees, 'merkleIndex out of range');
    require(verifyMetadata(tree.metadataMerkleRoot, tokenId, uri, metadataProof), "The metadata proof could not be verified");

VoterID.sol:

    require (msg.sender == _owner_, 'Identity: Only owner may call this');
    require(msg.sender == _minter, 'Only minter may create identity');
    require(owners[thisToken] == address(0), 'Token already exists');
    require(ooner != address(0), 'No such token');
    require(isApproved(msg.sender, tokenId), 'Identity: Unapproved transfer');
    require(checkOnERC721Received(from, to, tokenId, data), "Identity: transfer to non ERC721Receiver implementer");
    require(isApproved(msg.sender, tokenId), 'Identity: Not authorized to approve');
    require(holder != approved, 'Identity: Approving self not allowed');
    require(holder != address(0), 'Identity: Invalid tokenId');
    require(owners[tokenId] == from, "Identity: Transfer of token that is not own");
    require(to != address(0), "Identity: transfer to the zero address");
    require(_index < numIdentities, 'Invalid token index');
    require(_index < balances[_address], 'Index out of range');
    require(_address != address(0), 'Cannot query zero address');

Tools Used

Manual Review / VSCode

It is recommended to add Custom Errors.

4. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleLib.sol:

MerkleEligibility.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

5. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleLib.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

FixedPricePassThruGate.sol:

SpeedBumpPriceGate.sol:

MerkleEligibility.sol:

MerkleIdentity.sol:

Tools Used

Manual Review / VSCode

It is recommended wrap incrementing of i with unchecked block:

for (uint256 i; i < numWarriors; ) {
  _mint(msg.sender);
  unchecked { ++i };
}

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

PermissionlessBasicPoolFactory.sol

MerkleLib.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

MerkleEligibility.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

It is recommended to remove explicit initializations with default values.

7. Pack integer values

Impact

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

Tools Used

Manual Review / VSCode

It is recommended to pack listed values in order to consume less storage and thus gas.

#0 - illuzen

2022-05-12T08:48:08Z

all duplicates

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter