code-423n4 / 2022-05-factorydao-findings

1 stars 1 forks source link

Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies #253

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L68-L79 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L107-L123 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L80-L91 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L137-L149 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L180-L202

Vulnerability details

Impact

Wrong bookkeeping, albeit limited to the concerned tree with a FoT Token Wrong amount emitted

Proof of Concept

contracts/MerkleDropFactory.sol:
  77:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed"); 

contracts/MerkleResistor.sol:
  121:         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"); 

contracts/PermissionlessBasicPoolFactory.sol:
  144:             success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
  198:         bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);

Arbitrary ERC20 tokens can be passed (even malicious ones, rendering the tree malicious, as stated multiple times across the solution in comments).

With a transfer, the received amount should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

illuzen commented 2 years ago

Duplicate #34