Users Can Prevent Excess Tokens From Being Withdrawn By The Pool Creator In `withdrawExcessRewards()`

[code423n4]

Lines of code

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

Vulnerability details

Impact

Because pools will likely never be fully utilised by stakers while active, the following assumption in withdrawExcessRewards() can be broken by preventing any receipt withdrawal:

require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');

There are two main ways that this assumption can be broken:

• If a reward token attaches some callback (as found in the ERC777 and ERC1155 token standards), a receipt owner can refuse to accept a transfer by actively reverting when receiving any funds. The callback gives control to the recipient over how they receive funds. This can be actively used to deny all calls to withdrawExcessRewards(), leaving pool tokens locked up forever.
• A user deposits a single wei into a pool multiple times over using different accounts. Even though any user can call withdraw() on a given receipt (assuming the pool's active period has ended), a deposit can be extended over any number of accounts to make it incredibly difficult for the excess beneficiary to iterate and withdraw on behalf of these accounts.

Recommended Mitigation Steps

Make use of a grace period after a pool's staking period ends. This should give ample time to stakers to make the necessary withdrawal on the pool. Subsequently, the withdrawExcessRewards() function should be callable by anyone, allowing all funds unused funds to be withdrawn.

[illuzen]

Duplicate #20

[gititGoro]

reducing severity as rewards are not base assets.

[0xleastwood]

Pretty sure this is different. A single user could prevent excess rewards from being withdrawn. IMO, this is not a dupe of #20