Submitted by Jeiwan, also found by dev0cloo (1, 2), jaraxxus, 0xSmartContract, oxchsyston, 0xStalin (1, 2, 3), keccak123, dirk_y, btk, neumo, grearlake, 3docSec, rvierdiiev, and ABAIKUNANBAEV
A malicious vault can be deployed via the official VaultFactory contract. Users may lose funds while interacting with such vaults.
The VaultFactory.deployVault function allows deploying Vault contracts. The factory contract maintains a mapping to verify that a vault has been deployed via the factory. This allows users to check the authenticity of a vault to ensure the that implementation of a vault is authentic (i.e. not altered/malicious).
However, the business logic of vaults is split into multiple contracts: Vault, TwabController, and PrizePool. TwabController tracks the historical balances of users to determine their chances of winning prizes. PrizePool runs regular draws and distributes prizes among winners. Thus, its critical that in every authentic Vault contract, the implementations of TwabController and PrizePool are also authentic. Otherwise, a malicious actor could deploy an authentic vault via the official VaultFactory and provide malicious TwabController and PrizePool contracts; which can incorrectly determine user balances, favors some specific addresses when determining winners, or steals the prize token. In the current implementation, users are be able to check the authenticity of a vault contract, but not the authenticity of the TwabController and the PrizePool contracts that a vault integrates with.
Consider implementing TwabController and PrizePool factory contracts. In the contracts, consider tracking the addresses of the deployed TwabController and PrizePool contracts. In the VaultFactory.deployVault function, consider checking that the passed _twabController and _prizePool address were deployed via the respective factory contracts.
Picodes (judge) commented:
asselstine (PoolTogether) acknowledged and commented via duplicate issue #324:
