PrivatePool ERC-20 allowance stealing via execute()

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459

Vulnerability details

Impact

In PrivatePool, there are functions that assume the trader has approved the contract to spend their funds. For example, these functions are buy(), change(), and deposit():

• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L256
• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L423-L426
• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L502

However, these functions do not reset the allowance based on the results of the trades. The allowance can be greater than the commission requires, up to infinity. For example, some DeFi projects on Ethereum Mainnet set a request on the frontend to sign a transaction for infinite allowance so that the user does not pay unnecessary fees next time they interact with the project.

The pool owner can use the execute() function to call ERC20(baseToken).transferFrom(victimAddress, maliciousOwner, anyAmountOfTokens) on behalf of the pool, which has the approval from the victim to transfer their funds:

• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459

It should be noted that there is no implemented router for PrivatePool with ERC-20 base token. Therefore, interaction with such private pools is currently possible only directly, which makes the importance of the discovery HIGH.

Recommended Mitigation Steps

It is recommended to remove the execute() function and reconsider the way airdrops are received, or to clear the trader's allowance at the end of the transactions.

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #184

[c4-judge]

GalloDaSballo marked the issue as satisfactory