FactoryDAO contest - mtz's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L249-L254 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L248-L254 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L265-L271

Vulnerability details

Impact

Reward token transfers are done in 3 functions withdraw, withdrawExcessRewards and withdrawTaxes. If transfer of any reward token returns False or reverts for whatever reason these functions completely fail. This can lead to loss of funds for 3 kinds of users. 1. Depositers can lose their deposit and any reward tokens they are entitled to. 2. The globalBeneficiary could lose the taxes for a pool that they are entitled to. 3. The pool funder could lose any excess reward tokens they have used to fund a pool.

All of the above cases are possible if a token fails to transfer. But the first two, depositers and globalBenficiary ,can be harmed by an intentionally malicious pool funder. This malicious funder can create a malicious token contract and add that token to a pool. This token contract can fail to transfer when the attacker toggles a switch, causing the globalBeneficiary not to be able to receive taxes for that pool, and more importantly for depositers to lose their deposit and any reward tokens they are entitled to.

Note that pool funders can also fall victim to this problem if one of the reward tokens accidentally malfunctions. Because this is less likely than a token intentionally malfunctioning, it is a less serious concern.

Proof of Concept

1. Attacker creates an ERC20 token that has functionality allowing the attacker to blacklist certain addresses, causing transfers to those addresses to fail.
2. Attacker creates a pool and adds to it a reward token such as WETH (to attract victims/users) and their malicious token created in step 1.
3. Victim/user deposits depositToken into evil pool.
4. Attacker tells token to blacklist the victim depositer and globalBenficiary
5. Victim/user tries to withdraw their depositToken from evil pool. The transfer of the the depositToken and WETH would succeed but because the transfer of the evil token reverts, the entire call to withdraw reverts and the victim gets back nothing.
6. The globalBeneficiary cannot withdraw any taxes from that pool.

Recommended Mitigation Steps

This flaw can harm depositers, the globalBeneficiary and pool funders. The harm is most serious to depositers who can even lose their deposit. It is less serious to the globalBeneficary who can lose taxes for a single pool and it is least harmful to pool funders who will lose excess deposits (but a malicious pool funder cannot use this to attack them, harm can only result if one of the token genuinely fails to transfer not accidentally).

There are a number of ways to mitigate this problem.

1. Create an emergencyWithdraw function that allows depositers to withdraw their depositToken without receiving any reward tokens.
2. Create versions of withdraw (for rewards), withdrawTaxes, and withdrawExcessTokens that allow callers to specify which token they want to withdraw. This way they can get the functioning tokens they are entitled to while only losing the malfunctioning token (there is no way to prevent theft of an attacker controlled token anyway).