FactoryDAO contest - z3s's results

Lines of code

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

Vulnerability details

Impact

PermissionlessBasicPoolFactory.sol Consider this function:

function deposit(uint poolId, uint amount) external {
    Pool storage pool = pools[poolId];
    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');
    if (pool.totalDepositsWei + amount > pool.maximumDepositWei) {
        amount = pool.maximumDepositWei - pool.totalDepositsWei;
    }
    pool.totalDepositsWei += amount;
    pool.numReceipts++;

    Receipt storage receipt = pool.receipts[pool.numReceipts];
    receipt.id = pool.numReceipts;
    receipt.amountDepositedWei = amount;
    receipt.timeDeposited = block.timestamp;
    receipt.owner = msg.sender;

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

    emit DepositOccurred(poolId, pool.numReceipts, msg.sender);
}

if pool.depositToken === <USDT_ADDRESS> this function will revert because transferFrom of usdt does'nt return anything. and IERC20.transferFrom want a boolean.

other places with same problem:

MerkleDropFactory.sol
  77,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  107,42:         require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");

MerkleResistor.sol
  121,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  204,42:         require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');

MerkleVesting.sol
  89,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  173,34:         IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

PermissionlessBasicPoolFactory.sol
  144,62:             success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount); ///
  198,49:         bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
  230,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
  233,55:         success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
  252,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(pool.excessBeneficiary, rewards);
  269,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20 safeTransfer/safeTransferFrom instead of transfer/transferFrom that handles the return value check as well as non-standard-compliant tokens.

Gas Optimizations

[G01] Use calldata instead of memory:

For external function's dynamic params, calldata is the cheapest location to use.

MerkleDropFactory.sol
  88,82:     function withdraw(uint treeIndex, address destination, uint value, bytes32[] memory proof) public {

MerkleEligibility.sol
  70,66:     function isEligible(uint index, address recipient, bytes32[] memory proof) public override view returns (bool eligible) {
  85,68:     function passThruGate(uint index, address recipient, bytes32[] memory proof) external override {

MerkleIdentity.sol
  116,62:     function withdraw(uint merkleIndex, uint tokenId, string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof) external payable {
  116,84:     function withdraw(uint merkleIndex, uint tokenId, string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof) external payable {
  116,115:     function withdraw(uint merkleIndex, uint tokenId, string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof) external payable {
  152,72:     function isEligible(uint merkleIndex, address recipient, bytes32[] memory proof) public view returns (bool) {
  163,64:     function verifyMetadata(bytes32 root, uint tokenId, string memory uri, bytes32[] memory proof) public pure returns (bool) {
  163,86:     function verifyMetadata(bytes32 root, uint tokenId, string memory uri, bytes32[] memory proof) public pure returns (bool) {

MerkleLib.sol
  17,64:     function verifyProof(bytes32 root, bytes32 leaf, bytes32[] memory proof) public pure returns (bool) {

MerkleResistor.sol
  134,136:     function initialize(uint treeIndex, address destination, uint vestingTime, uint minTotalPayments, uint maxTotalPayments, bytes32[] memory proof) external {

MerkleVesting.sol
  104,143:     function initialize(uint treeIndex, address destination, uint totalCoins, uint startTime, uint endTime, uint lockPeriodEndTime, bytes32[] memory proof) external {

interfaces/IEligibility.sol
  17,50:     function isEligible(uint, address, bytes32[] memory) external view returns (bool eligible);
  21,52:     function passThruGate(uint, address, bytes32[] memory) external;

Recommended Mitigation Steps:

Change params memory to calldata

[G02] uint default value is 0 so we can remove = 0:

MerkleDropFactory.sol
  17,26:     uint public numTrees = 0;

MerkleEligibility.sol
  31,26:     uint public numGates = 0;

MerkleLib.sol
  22,21:         for (uint i = 0; i < proof.length; i += 1) {

MerkleResistor.sol
  24,26:     uint public numTrees = 0;
  176,32:         uint currentWithdrawal = 0;

MerkleVesting.sol
  16,26:     uint public numTrees = 0;
  150,32:         uint currentWithdrawal = 0;

PermissionlessBasicPoolFactory.sol
  115,21:         for (uint i = 0; i < rewardTokenAddresses.length; i++) {
  141,21:         for (uint i = 0; i < pool.rewardFunding.length; i++) {
  168,21:         for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
  224,21:         for (uint i = 0; i < rewards.length; i++) {
  249,21:         for (uint i = 0; i < pool.rewardTokens.length; i++) {
  266,21:         for (uint i = 0; i < pool.rewardTokens.length; i++) {

VoterID.sol
  69,31:     uint public numIdentities = 0;

[G03] ++i use less gas than i++ or i += 1:

++i costs less gas compared to i++. about 5 gas per iteration.

PermissionlessBasicPoolFactory.sol
  115,59:         for (uint i = 0; i < rewardTokenAddresses.length; i++) {
  141,57:         for (uint i = 0; i < pool.rewardFunding.length; i++) {
  168,71:         for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
  224,46:         for (uint i = 0; i < rewards.length; i++) {
  249,56:         for (uint i = 0; i < pool.rewardTokens.length; i++) {
  266,56:         for (uint i = 0; i < pool.rewardTokens.length; i++) {

MerkleLib.sol
  22,21:         for (uint i = 0; i < proof.length; i += 1) {

[G04] Use Custom Errors to save Gas:

Custom errors from Solidity 0.8.4 are cheaper than require messages. https://blog.soliditylang.org/2021/04/21/custom-errors/