FactoryDAO contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Throughout your contracts, you expect every ERC20 to return a boolean value on transfers, e.g.

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

While it's specified like that in the EIP, there are contracts that don't follow that standard. One prominent example is the USDT token: https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L85

Because of that, there is the SafeERC20 library. There the transfer function only verifies the return value if there is one, see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L82-L98

There are two transfer functions you call in your contracts:

• transferFrom() -> used whenever funds are deposited from the caller to the contract
• transfer() -> used whenever funds are sent to the user from the contract

In the case of USDT, neither of the functions returns a bool. That token will simply not be usable with your contracts. For example, in PermissionlessBasicPoolFactory the creator of a pool won't be able to deposit the rewards tokens if one of them is USDT. But, if the return value is only missing in the token's transfer() function you have a real problem. Users would be able to deposit their tokens since that uses transferFrom() but won't be able to get them back from the contracts.

Proof of Concept

Here's a small test contract using foundry that highlights the issue:

pragma solidity ^0.8.12;

import "forge-std/Test.sol";

import "src/IERC20.sol";

contract BrokenERC20 {
  function transfer(address to, uint amount) external {}
}

contract TestContract is Test {
    function testB() external {
      BrokenERC20 t = new BrokenERC20();
      require(IERC20(address(t)).transfer(address(this), 1));
    }
}

testB() will always fail because IERC20(address(t)).transfer(address(this), 1) will always return the zero value of a bool which is false.

Here are all the instances where the transfer functions have to be updated:

./contracts/PermissionlessBasicPoolFactory.sol:255:            success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
./contracts/PermissionlessBasicPoolFactory.sol:258:        success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
./contracts/PermissionlessBasicPoolFactory.sol:278:            success = success && IERC20(pool.rewardTokens[i]).transfer(pool.excessBeneficiary, rewards);
./contracts/PermissionlessBasicPoolFactory.sol:296:            success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);
./contracts/MerkleDropFactory.sol:110:        require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");
./contracts/MerkleResistor.sol:206:        require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');
./contracts/MerkleVesting.sol:173:        IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);
./contracts/PermissionlessBasicPoolFactory.sol:146:            success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
./contracts/PermissionlessBasicPoolFactory.sol:213:        bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
./contracts/MerkleDropFactory.sol:79:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
./contracts/MerkleResistor.sol:123:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
./contracts/MerkleVesting.sol:89:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");

Tools Used

none

Recommended Mitigation Steps

Use SafeERC20's transfer functions

Lines of code

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

Vulnerability details

Impact

PermissionlessBasicPoolFactory contract not handling fee-on-transfer tokens properly can cause a loss of funds for users.

Since anybody can create a pool and decide which tokens are supposed to be used for the deposits and the reward, there's the possibility of someone creating one with a token that takes fees on transfer. In that case, the internal balance of the contract would be wrong. It would think that it had more tokens than it actually does. When users start withdrawing their deposits & rewards, the person who withdraws last will not be able to withdraw their deposit & reward.

Because of that, I'd rate this issue HIGH. Especially considering the fact that the time gap between deposits and the first withdrawal will be quite large. The pool could fill up before users realize that the contract can't handle fee-on-transfer tokens.

NOTE: the same problem exists for all the other contracts transferring ERC20 tokens. But, in those cases, the user should be able to save funds by modifying the Merkle tree.

Proof of Concept

When depositing tokens, the token doesn't verify that it received the correct amount: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L194-L198

When withdrawing, it uses the value stored when the user deposited: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L233

If it's a fee-on-transfer token that might be more than the contract actually owns.

Basic example:

• F-Token takes a 1% fee per transfer
• Alice deposits 1e18 tokens
• Contract only receives 1e16 tokens
• Alice tries to withdraw tokens
• Contract tries to send Alice 1e18 tokens since that's the amount stored in amountDepositedWei. The transaction fails because the contract doesn't have 1e18 tokens. Alice can't withdraw her funds.

Tools Used

none

Recommended Mitigation Steps

Store the actual token amount the contract receives, e.g.:

uint prevBalance = IERC20(pool.depositToken).balanceOf(address(this));
IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
uint postBalance = IERC20(pool.depositToken).balanceOf(address(this));
receipt.amountDepositedWei = postBalance - prevBalance;

Report

• emit event on configuration changes
• use two-step process for critical address changes
• VoterID contract doesn't follow ERC721 specs

emit event on configuration changes

Whenever you make a change to the contract's configuration there should be an event.

Here are a couple functions where that's missing:

• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L61
• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L68
• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L77
• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L317
• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L163

use two-step process for critical address changes

Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.

• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L153
• https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L61

Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

VoterID contract doesn't follow ERC721 specs

The contract isn't fully compliant with the ERC721-specs.

balanceOf() should throw if an invalid address is passed.

tokenURI() should throw if an invalid tokenID is passed.

Cache reads from storage to save gas

for (uint i = 0; i < pool.rewardTokens.length; i++) {
  // ...
}

// change to

uint length = pool.rewardTokens.length;
for (uint i = 0; i < length; i++) {
  // ...
}