Malicious token reward could disable withdrawals

[code423n4]

Lines of code

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

Vulnerability details

Impact

PermissionlessBasicPoolFactory.withdraw requires each reward token transfers to succeed before withdrawing the deposit. If one of the reward token is a malicious/pausable contract that reverts on transfer, unaware users that deposited into this pool will have their funds stuck in the contract.

Recommended Mitigation Steps

Add an emergencyWithdraw function that ignores failed reward token transfers.

[illuzen]

This is explicitly acknowledged in the contract comments, malicious reward tokens render the pool malicious, there is no way to get around that, but the emergencyWithdraw idea is good

[illuzen]

Technically duplicate, but mitigation is better here.

[illuzen]

code-423n4/2022-05-factorydao#2

[ksk2345]

1. User Funds are at loss so severity of this issue is High. Please check the descriptions in the duplicate IssueIDs : In case of any malicious rewardToken contract, withdraw of deposit will also fail #191, Staking pool withdrawals will totally fail if one reward token withdrawal fails #145, and PermissionlessBasicPoolFactory: Malicious reward tokens can brick withdrawals #106 (marked wrongly as dup of M13)
2. IssueId If multiple reward tokens configured, its possible to block withdrawTaxes of all rewardTokens #192 and Pool Creators Can Reject Taxes From Being Withdrawn #246 are wrongly marked as dup of M02. The root cause is malicious rewardToken, but the impact is different than that of M02, and also the code fix for the impact will be different. M02 talks of impact on loss of user deposits and the fix will be in withdraw function. While these two Issues impact on globalBeneficiary not able to withdraw rewards, the fix will be in withdrawTaxes() function. Hence, these two issues needs to be separted into a New Medium Issue.

[gititGoro]

1. User Funds are at loss so severity of this issue is High. Please check the descriptions in the duplicate IssueIDs : In case of any malicious rewardToken contract, withdraw of deposit will also fail #191, Staking pool withdrawals will totally fail if one reward token withdrawal fails #145, and PermissionlessBasicPoolFactory: Malicious reward tokens can brick withdrawals #106 (marked wrongly as dup of M13)
2. IssueId If multiple reward tokens configured, its possible to block withdrawTaxes of all rewardTokens #192 and Pool Creators Can Reject Taxes From Being Withdrawn #246 are wrongly marked as dup of M02. The root cause is malicious rewardToken, but the impact is different than that of M02, and also the code fix for the impact will be different. M02 talks of impact on loss of user deposits and the fix will be in withdraw function. While these two Issues impact on globalBeneficiary not able to withdraw rewards, the fix will be in withdrawTaxes() function. Hence, these two issues needs to be separted into a New Medium Issue.

I really do sympathize with your point of view. I think you made two good points.
Here's the thing, though: When assessing these issues, it's very important to take sponsor's intent into account. That's what makes humans necessary in C4 (for now). Sufficiently powerful software can form a graph of vulnerabilities and draw inferences. Our job is to figure out if these vulnerabilities matter and to what extent.

First to the broader point of linking duplicates, whether blocking users or taxes, the vector is via a malicious pool creator in the form of a reward token. The reason this matters is because the Factorydao pools can be permissionlessly created and completely ignored by users. They are analogous to Uniswap pools. Sure, someone could create a malicious token pair in Uniswap but since all pairs are opt-in and can be routed around, the use of this pair requires explicit consent from the end user.

If we bear in mind that end user involvement is consensual and that this is communicated to the users and that nothing can be hidden on Ethereum then it follows that we can umbrella these issues not as malicious tokens or withdrawal and tax vulnerabilities but pool creators trying to game the code while relying on social engineering to funnel unsuspecting users through these channels (scam).

digression on duplicates

On a broader issue of duplicates, I notice a similar theme arising when duplicate labels are challenged so I'd just like to clarify how I grouped duplicates:

For most of the duplicate disputes, it was "the issue reported the same but the fix was different." If the issue is invalid, however, then the fix is kind of irrelevant which is why I grouped those all as the same issue. If the issue is the same but the fixes different, then I grouped them if the fixes were qualitatively similar. If there was one amongst them that provided the best fix, this would be the original in the set.