FactoryDAO contest - VAD37's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L252 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L198 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L230 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L233 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L252 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L269 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleDropFactory.sol#L77 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleDropFactory.sol#L107 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L89 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L173 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleResistor.sol#L121 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleResistor.sol#L204

Vulnerability details

Token like USDT known for using non-standard ERC20. (Missing return boolean on transfer).

It does not matter if require() is used or not. Interface call to these token will always be reverted due to incompatible interface.

Impact

Failed interact with non-standard ERC20 tokens especially PermissionlessBasicPoolFactory.addPool cannot accept USDT as funding or rewards token.

Unexpected contract functionality = Medium severity

Migration

Use SafeTransferLib.safeTransfer or similar library instead of IERC20 transfer. This bypass ERC20 token with no boolean return like USDT.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L142-L146

Vulnerability details

For the Meme token, this issue also extend to MerkleDrop rewards but not serious issue since owner can transfer more rewards than required to cover burn expense on first transfer.

Impact

For token tax on transfer, there are many case where not enough rewards in pool funding to call withdrawExcessRewards and withdrawTaxes.

Proof Of Concept

• globalTaxPerCapita is 0.5%.
• Meme token burn 10% on transfer.
• The amount received by pool is 90% of funding amount.
• There is no issue for people to withdraw their rewards.
• When the pool excessBeneficiary try to withdrawExcessRewards but see the excess amount might not enough to cover the original burn expense. Transfer extra 11% token to the pool to add the pool amount = original amount.
• Now the pool excessBeneficiary can withdraw all tokens.
• Only the globalBeneficiary take the hit because the 0.5% tax amount does not exist in the pool anymore.

Recommendation

Store token balance different in fundPool.

for (uint i = 0; i < pool.rewardFunding.length; i++) {
    amount = getMaximumRewards(poolId, i); 
    // transfer the tokens from pool-creator to this contract
    IERC20 token = IERC20(pool.rewardTokens[i]);
    uint before = token.balanceOf(address(this));            
    token.safeTransferFrom(msg.sender, address(this), amount);
    uint after = token.balanceOf(address(this));
    uint diff = after - before;
    require( diff <= amount, "Prevent reentrancy manipulate pool funding");
    // bookkeeping to make sure pools don't share tokens
    pool.rewardFunding[i] += diff; 
}

The issue also affect deposit function. User deposit less amount than pool received. Rewards still the same might not cover burn expense.

The true damage extend of meme token tax on transfer in wider system is unknown. I would recommend ignore all these tokens all together and accept loss.

Lines of code

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

Vulnerability details

Hardcode 1e18 for all rewards token can cause addPool issue with rewards/reposit token like WBTC (8 decimals) or USDC (6 decimals). The underlying function getMaximumRewards will return 0 or smaller rewards than expected due to precision lost.

Impact

Normally this would not be a problem because maximumDepositWei or rewardsWeiPerSecondPerToken will be token with 18 decimals large enough to handle hardcode 1e18 division.

But if user want user to deposit WBTC and rewards both USDC and promoted token (either with 18 decimals or not), then this issue will occur. (Yield reward per second for USDC is really high but received rewards is 0)

Unexpected contract behaviour + Hypothetical scam = Medium severity.

Proof Of Concept

1. Add pool with MyToken(18 decimals) and USDC(8 decimals) as token rewards for depositing WBTC.
2. Max deposit WBTC should be 10 WBTC (1e9) over the period of 1 month (2592000 ~= 2e6). Rewards 100K USDC (1e12) rewardsWeiPerSecondPerToken = 1e12 / 2e6 ~= 5e5.
3. During fundPool call. MyToken have no problem, but getMaximumRewards for USDC will be calculated as 5e5 * 1e9 * 2e6 / 1e18 = 1e21 / 1e18 = 1000 = 0.001 USDC.
4. Only 0.001 USDC is transfered as reward for the pool. Plus, lots of MyToken that might be worth something.

The scam use case for this pretty much inflated price for meme/DAO token. Since no USDC is rewarded, the scammer only have to mint their token to give out. Little expense, free exposure. No user lost their WBTC.

Migitation

The easiest way is replacing hardcode / 1e18 with depositToken.decimals(). Note: still not handle meme token case like there is only 1 token with only 1e18 in circulation.

This is up to user to handle.

QA

1. QA
   1. Reentrancy can change metadata
   2. Unchecked ERC20 Transfer
   3. missing zero-check
   4. Missing Event
   5. Inconsistent Solidity pragma version
   6. Typo ooner
   7. addPool require() should be on top of function
   8. MerkleResistor.initialize unnecessary variable destination
   9. Confusing message error

Most of issues caught by Slither filter by me, while keep the original format. Beware of blue formated text "<text>" as low (or med) issue should be fixed, everything else is optional as non-critical.

If Github format does not work correctly. Just use search for "<>" symbol to find issue should be fixed.

Reentrancy can change metadata

PermissionlessBasicPoolFactory.addPool reentrancy through fundPool function. Can overwritten metadata for same pool (id based on ++numPools) multiple time.

Can cause transaction emit event in wrong order with pool ID and name mismatch with stored metadata.

Only cause issue with frontend, database or tools based on transaction events instead of reading metadata from blockchain directly.

Unchecked ERC20 Transfer

<MerkleVesting.withdraw>(uint256,address) (MerkleVesting.sol#139-175) ignores return value by IERC20(tree.tokenAddress).transfer(destination,currentWithdrawal) (MerkleVesting.sol#173)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-transfer

Interface call still revert if external contract call revert, but who known what weird token user want. It is recommended to use library SafeTransfer instead of interface call.

missing zero-check

MerkleEligibility.constructor(address)._gateMaster (MerkleEligibility.sol#35) lacks a zero-check on :
  - gateMaster = _gateMaster (MerkleEligibility.sol#36)
MerkleIdentity.constructor(address)._mgmt (MerkleIdentity.sol#52) lacks a zero-check on :
  - management = _mgmt (MerkleIdentity.sol#53)
  - treeAdder = _mgmt (MerkleIdentity.sol#54)
<MerkleIdentity.setManagement>(address).newMgmt (MerkleIdentity.sol#60) lacks a zero-check on :
  - management = newMgmt (MerkleIdentity.sol#61)
MerkleIdentity.setTreeAdder(address).newAdder (MerkleIdentity.sol#67) lacks a zero-check on :
  - treeAdder = newAdder (MerkleIdentity.sol#68)
PermissionlessBasicPoolFactory.constructor(address,uint256)._globalBeneficiary (PermissionlessBasicPoolFactory.sol#77) lacks a zero-check on :
  - globalBeneficiary = _globalBeneficiary (PermissionlessBasicPoolFactory.sol#78)
VoterID.constructor(address,address,string,string).ooner (VoterID.sol#108) lacks a zero-check on :
  - _owner_ = ooner (VoterID.sol#109)
VoterID.constructor(address,address,string,string).minter (VoterID.sol#108) lacks a zero-check on :
  - _minter = minter (VoterID.sol#111)
<VoterID.setOwner>(address).newOwner (VoterID.sol#151) lacks a zero-check on :
  - _owner_ = newOwner (VoterID.sol#153)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation

Changing owner and management can have accident with javascript where the address is not properly validated (use null address).

Missing Event

<MerkleIdentity.setManagement>(address) (MerkleIdentity.sol#60-62) should emit an event for: 
 - management = newMgmt (MerkleIdentity.sol#61) 
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-access-control

<PermissionlessBasicPoolFactory.setGlobalTax>(uint256) (PermissionlessBasicPoolFactory.sol#316-320) should emit an event for: 
 - globalTaxPerCapita = newTaxPerCapita (PermissionlessBasicPoolFactory.sol#319) 
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-arithmetic

Other external function missing events not picked up by slither include:

• MerkleIdentity.setTreeAdder(address)
• MerkleIdentity.setIpfsHash(uint,bytes32)
• MerkleIdentity.withdraw(uint,uint,string,bytes32[],bytes32[]) should at least emit final event
• FixedPricePassThruGate.PassThruGate(uint,address)
• SpeedBumpPriceGate.PassThruGate(uint,address)
• MerkleEligibility.(uint,address,bytes32[])

Inconsistent Solidity pragma version

PermissionlessBasicPoolFactory.sol use 0.8.12 while other file use 0.8.9. Current recommended version still 0.8.4 - 0.8.7

Typo ooner

should be owner

addPool require() should be on top of function

User call addPool call lots of unnecessary code before reaching the revert point. It also save gas for user too.

MerkleResistor.initialize unnecessary variable destination

MerkleResistor require destination = msg.sender on top of function. There is no interface require destination for function. Why not just use msg.sender?

Confusing message error

The message does not tell user/dev do the positive or opposite action of the message.

Gas

1. Gas
   1. Discrepancy Increment use
   2. change unused public function to external

Discrepancy Increment use

• MerkleLib.verifyProof use i++
• VoterId.createIdentityFor change to allTokens[numIdentities++] = thisToken;
• Current store VoterId mapping is way optimal than standard balances[thisOwner]++ . If change to ++balances[thisOwner], it can save extra 3 gas than current version.

change unused public function to external

Issue copy pasted directly from slither. External function call cost less gas than public function.

• MerkleDropFactory.addMerkleTree(bytes32,bytes32,address,uint256) (MerkleDropFactory.sol#49-62)
• MerkleDropFactory.withdraw(uint256,address,uint256,bytes32[]) (MerkleDropFactory.sol#88-109)
• MerkleIdentity.getPrice(uint256) (MerkleIdentity.sol#140-144)
• MerkleIdentity.isEligible(uint256,address,bytes32[]) (MerkleIdentity.sol#152-155)
• MerkleLib.verifyProof(bytes32,bytes32,bytes32[]) (MerkleLib.sol#17-29)
• MerkleResistor.addMerkleTree(bytes32,bytes32,uint256,uint256,uint256,address,uint256) (MerkleResistor.sol#80-101)
• MerkleVesting.addMerkleRoot(bytes32,bytes32,address,uint256) (MerkleVesting.sol#62-73)
• VoterID.createIdentityFor(address,uint256,string) (VoterID.sol#122-144)
• VoterID.safeTransferFrom(address,address,uint256) (VoterID.sol#226-228)
• VoterID.isApprovedForAll(address,address) (VoterID.sol#270-272)
• VoterID.owner() (VoterID.sol#417-419)