Lines of code

Vulnerability details

Impact

Function deposit() of PermissionlessBasicPoolFactory supposed to revert if user send uninitialized poolId by mistake, but if user does this, attacker can perform front-running attack and create multiple pools with his smart contract and be owner of that poolId and make user funds to be locked as long as attacker wants. the root cause is that poolId is not enough for identifying user specified pool.

Proof of Concept

This is deposit() code:

    function deposit(uint poolId, uint amount) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');
        require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
        require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
        require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');

As you can see, the code will revert if the specified poolId is not initialized and users expect this behavior that if they send wrong poolId the transaction will result in revert. but attacker can interfere and make user to deposit his tokens to malicious pool created by attacker to perform this attacker will do this:

1. attacker will create a smart contract that calls addPool of PermissionlessBasicPoolFactory for specified times and specified params.

2. attacker create a bot and will watch new transactions to find a user’s transaction that calls deposit() with wrong poolId and poolId is not so much higher than numPools.

3. attacker’s bot will create a transaction with higher gas price which calls attacker’s contract and the contract will create numPools-poolId pools in PermissionlessBasicPoolFactory with attacker specified parameters. (the new pools will set depositTokenAddress as one of tokens that user approved for PermissionlessBasicPoolFactory).

4. becasue of this line in the deposit() user transaction need to be mined in next block, so attacker had to take some measures to force user transaction to be mined later.

      require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
   

5. after attacker contract created malicious pools and specified parameters for the poolId with front-running attack, then user transaction will be mined and contract will accept that transaction and it will transfer user tokens to contract address(attacker can choose which token).

      Receipt storage receipt = pool.receipts[pool.numReceipts];
      receipt.id = pool.numReceipts;
      receipt.amountDepositedWei = amount;
      receipt.timeDeposited = block.timestamp;
      receipt.owner = msg.sender;
   
      bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
      require(success, 'Token transfer failed');
   
      emit DepositOccurred(poolId, pool.numReceipts, msg.sender);
   

6. because attacker specified his own erc20 tokens as reward tokens, he can make withraw() function for that poolId to revert and user will not able to withdraw his funds ever unless he pays attackers.

Tools Used

VIM

Recommended Mitigation Steps

depositing by specifying only poolId is not secure, users should send excessBeneficiary too, and contract can check that if that poolId and excessBeneficiary is not a match and excessBeneficiary of that poolId is 0x0 then revert. in this way attacker can nevert intrupt user’s transactions with wrong values.

The text was updated successfully, but these errors were encountered:

All reactions