Re-entrancy attack on the main functions

[code423n4]

Lines of code

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

Vulnerability details

Impact

A malicious token, or one that implemented transfer hooks, could re-enter the public calling function (such as withdraw()) before proper internal accounting was completed. Because the earned reward function looks up the pool.totalDepositsWei and pool.rewardsWeiClaimed[, which is not yet updated when the transfer occurs, it would be possible for a malicious contract to re-enter _withdraw repeatedly and drain the pool. (Tokens with hooks (ERC777 and ERC677) would allow to exploit the contract and drain it in it's entirety.)

Proof of Concept

1. Navigate to the following contract.

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

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

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

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

2. withdraw, deposit and other functions are missing re-entrancy guard. Therefore, If pool is added with ERC777 token, the attacker can drain all pool.

Tools Used

Code Review

Recommended Mitigation Steps

Consider using re-entrancy guard on all main action functions (e.g. deposit, withdraw and etc): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

[illuzen]

ReentrancyGuard is not the only way to guard against re-entrance. Please read the code.

Duplicate #34

[KenzoAgada]

Doesn't seem to be duplicate of #34 to me. 34 is about FoT tokens and checking balance before and after. This issue is a general reentrancy warning issue. There's no valid attack path outlined, and as the sponsor says, the code is protected via other means. Should probably be invalid or low severity.

[gititGoro]

The link to 34 was made because they're both about state update issues in the presence of potential reentrancy or token state updates that disagree with the parameters. FOT is just one example of a non malicious token which is why I added that footnote comment. In both issues, there wasn't a solid attack path outlined but the sponsor was concerned with the general issue of handling unorthodox token transfers. This is why the issue remained confirmed and in fact a pull request was submitted addressing it.