PoolTogether - Pods

Description

Pod.depositTo() grants users shares of the pod pool in exchange for tokenAmount of token.

code/pods-v3-contracts/contracts/Pod.sol:L266-L288

function depositTo(address to, uint256 tokenAmount)
    external
    override
    returns (uint256)
{
    require(tokenAmount > 0, "Pod:invalid-amount");

    // Allocate Shares from Deposit To Amount
    uint256 shares = _deposit(to, tokenAmount);

    // Transfer Token Transfer Message Sender
    IERC20Upgradeable(token).transferFrom(
        msg.sender,
        address(this),
        tokenAmount
    );

    // Emit Deposited
    emit Deposited(to, tokenAmount, shares);

    // Return Shares Minted
    return shares;
}

The winner of a prize pool is typically determined by an off-chain random number generator, which requires a request to first be made on-chain. The result of this RNG request can be seen in the mempool and frontrun. In this case, an attacker could identify a winning Pod contract and make a large deposit, diluting existing user shares and claiming the entire prize.

Recommendation

The modifier pauseDepositsDuringAwarding is included in the Pod contract but is unused.

code/pods-v3-contracts/contracts/Pod.sol:L142-L148

modifier pauseDepositsDuringAwarding() {
    require(
        !IPrizeStrategyMinimal(_prizePool.prizeStrategy()).isRngRequested(),
        "Cannot deposit while prize is being awarded"
    );
    _;
}

Add this modifier to the depositTo() function along with corresponding test cases.

Description

There are a lot of token transfers in the code, and most of them are just calling transfer or transferFrom without checking the return value. Ideally, due to the ERC-20 token standard, these functions should always return True or False (or revert). If a token returns False, the code will process the transfer as if it succeeds.

Recommendation

Use the safeTransfer and the safeTransferFrom versions of transfers from OZ.

Description

The TokenDrop.initialize() function is unprotected and can be called multiple times.

code/pods-v3-contracts/contracts/TokenDrop.sol:L81-L87

function initialize(address _measure, address _asset) external {
    measure = IERC20Upgradeable(_measure);
    asset = IERC20Upgradeable(_asset);

    // Set Factory Deployer
    factory = msg.sender;
}

Among other attacks, this would allow an attacker to re-initialize any TokenDrop with the same asset and a malicious measure token. By manipulating the balance of a user in this malicious measure token, the entire asset token balance of the TokenDrop contract could be drained.

Recommendation

Add the initializer modifier to the initialize() function and include an explicit test that every initialization function in the system can be called once and only once.

Description

During the deposit, the token transfer is made after the Pod shares are minted:

code/pods-v3-contracts/contracts/Pod.sol:L274-L281

uint256 shares = _deposit(to, tokenAmount);

// Transfer Token Transfer Message Sender
IERC20Upgradeable(token).transferFrom(
    msg.sender,
    address(this),
    tokenAmount
);

That means that if the token allows re-entrancy, the attacker can deposit one more time inside the token transfer. If that happens, the second call will mint more tokens than it is supposed to, because the first token transfer will still not be finished. By doing so with big amounts, it’s possible to drain the pod.

Recommendation

Add re-entrancy guard to the external functions.

Description

If the asset token is making a call before the transfer to the receiver or to any other 3-d party contract (like it’s happening in the Pod token using the _beforeTokenTransfer function), the attacker can call the drop function inside the transfer call here:

code/pods-v3-contracts/contracts/TokenDrop.sol:L139-L153

function claim(address user) external returns (uint256) {
    drop();
    _captureNewTokensForUser(user);
    uint256 balance = userStates[user].balance;
    userStates[user].balance = 0;
    totalUnclaimed = uint256(totalUnclaimed).sub(balance).toUint112();

    // Transfer asset/reward token to user
    asset.transfer(user, balance);

    // Emit Claimed
    emit Claimed(user, balance);

    return balance;
}

Because the totalUnclaimed is already changed, but the current balance is not, the drop function will consider the funds from the unfinished transfer as the new tokens. These tokens will be virtually redistributed to everyone.

After that, the transfer will still happen, and further calls of the drop() function will fail because the following line will revert:

uint256 newTokens = assetTotalSupply.sub(totalUnclaimed);

That also means that any transfers of the Pod token will fail because they all are calling the drop function. The TokenDrop will “unfreeze” only if someone transfers enough tokens to the TokenDrop contract.

The severity of this issue is hard to evaluate because, at the moment, there’s not a lot of tokens that allow this kind of re-entrancy.

Recommendation

Simply adding re-entrancy guard to the drop and the claim function won’t help because the drop function is called from the claim. For that, the transfer can be moved to a separate function, and this function can have the re-entrancy guard as well as the drop function.

Also, it’s better to make sure that _beforeTokenTransfer will not revert to prevent the token from being frozen.

Description

The Pod contract had the drop storage field and mapping of different TokenDrops (token => TokenDrop). When adding a new TokenDrop in the mapping, the drop field is also changed to the added _tokenDrop:

code/pods-v3-contracts/contracts/Pod.sol:L455-L477

function setTokenDrop(address _token, address _tokenDrop)
    external
    returns (bool)
{
    require(
        msg.sender == factory || msg.sender == owner(),
        "Pod:unauthorized-set-token-drop"
    );

    // Check if target<>tokenDrop mapping exists
    require(
        drops[_token] == TokenDrop(0),
        "Pod:target-tokendrop-mapping-exists"
    );

    // Set TokenDrop Referance
    drop = TokenDrop(_tokenDrop);

    // Set target<>tokenDrop mapping
    drops[_token] = drop;

    return true;
}

On the other hand, the measure token and the asset token of the drop are strictly defined by the Pod contract. They cannot be changed, so all TokenDrops are supposed to have the same asset and measure tokens. So it is useless to have different TokenDrops.

Recommendation

The mapping seems to be unused, and only one TokenDrop will normally be in the system. If that code is not used, it should be deleted.

Description

When withdrawing from the Pod, the shares are burned, and the deposit is removed from the Pod. If there are not enough deposit tokens in the contract, the remaining tokens are withdrawn from the pool contract:

code/pods-v3-contracts/contracts/Pod.sol:L523-L532

if (amount > currentBalance) {
    // Calculate Withdrawl Amount
    uint256 _withdraw = amount.sub(currentBalance);

    // Withdraw from Prize Pool
    uint256 exitFee = _withdrawFromPool(_withdraw);

    // Add Exit Fee to Withdrawl Amount
    amount = amount.sub(exitFee);
}

These tokens are withdrawn with a fee from the pool, which is not controlled or limited by the user.

Recommendation

Allow users to pass a maxFee parameter to control fees.

Description

The function ProxyFactory.deployMinimal() is used by both the PodFactory and the TokenDropFactory to deploy minimal proxy contracts. This function uses inline assembly to inline a target address into the minimal proxy and deploys the resulting bytecode. It then emits an event containing the resulting address and optionally makes a low-level call to the resulting address with user-provided data.

The result of a create() operation in assembly will be the zero address in the event that a revert or an exceptional halting state is encountered during contract creation. If execution of the contract initialization code succeeds but returns no runtime bytecode, it is also possible for the create() operation to return a nonzero address that contains no code.

code/pods-v3-contracts/contracts/external/ProxyFactory.sol:L9-L35

function deployMinimal(address _logic, bytes memory _data)
    public
    returns (address proxy)
{
    // Adapted from https://github.com/optionality/clone-factory/blob/32782f82dfc5a00d103a7e61a17a5dedbd1e8e9d/contracts/CloneFactory.sol
    bytes20 targetBytes = bytes20(_logic);
    assembly {
        let clone := mload(0x40)
        mstore(
            clone,
            0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000
        )
        mstore(add(clone, 0x14), targetBytes)
        mstore(
            add(clone, 0x28),
            0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000
        )
        proxy := create(0, clone, 0x37)
    }

    emit ProxyCreated(address(proxy));

    if (_data.length > 0) {
        (bool success, ) = proxy.call(_data);
        require(success, "ProxyFactory/constructor-call-failed");
    }
}

Recommendation

At a minimum, add a check that the resulting proxy address is nonzero before emitting the ProxyCreated event and performing the low-level call. Consider also checking the extcodesize of the proxy address is greater than zero.

Also note that the bytecode in the deployed “Clone” contract was not reviewed due to time constraints.

Description

The function Pod.setManager() allows the owner of the Pod contract to change the Pod’s manager. It checks that the value of the existing manager in storage is nonzero. This is presumably intended to ensure that the owner has provided a valid newManager parameter in calldata.

The current check will always pass once the contract is initialized with a nonzero manager. But, the contract can currently be initialized with a manager of IPodManager(address(0)). In this case, the check would prevent the manager from ever being updated.

code/pods-v3-contracts/contracts/Pod.sol:L233-L240

function setManager(IPodManager newManager)
    public
    virtual
    onlyOwner
    returns (bool)
{
    // Require Valid Address
    require(address(manager) != address(0), "Pod:invalid-manager-address");

Recommendation

Change the check to:

require(address(newManager) != address(0), "Pod:invalid-manager-address");

More generally, attempt to define validity criteria for all input values that are as strict as possible. Consider preventing zero inputs or inputs that might conflict with other addresses in the smart contract system altogether, including in contract initialization functions.

Description

The Pod contract contains an event Withdrawl(address, uint256, uint256):

code/pods-v3-contracts/contracts/Pod.sol:L76-L79

/**
 * @dev Emitted when user withdraws
 */
event Withdrawl(address user, uint256 amount, uint256 shares);

This appears to be a misspelling of the word Withdrawal. This is of course not a problem given it’s consistent use, but could cause confusion for users or issues in future contract updates.