Calm Fiery Llama - The graduator may not be able to withdraw funds from a graduated market which causes them to be stuck

[sherlock-admin2]

Calm Fiery Llama

High

The graduator may not be able to withdraw funds from a graduated market which causes them to be stuck

Summary

Whenever users buy votes for a reputation market, they have to pay a protocol fee. That protocol fee is sent to the protocolFeeAddress. However, the amount of protocol fees is still stored in the marketFunds mapping, which should only contain the funds currently invested in each market. As a result, when a market graduates and the graduator calls withdrawGraduatedMarketFunds() to withdraw the amount stored in marketFunds, the call could revert if the contract balance is less than the amount stored in the marketFunds mapping for that market due to the mapping also including fees.

Root Cause

In ReputationMarket.sol:481 the marketFunds will be increased by fundsPaid. The amount of fundsPaid is calculated in ReputationMarket::_calculateBuy() and includes the fees. The difference in contract balance and funds stored in the mapping causes calls to withdraw the funds from a graduated market to revert.

Internal pre-conditions

1. During any call to buy a vote, the entryProtocolFeeBasisPoints need to be greater than 0.

External pre-conditions

None.

Attack Path

1. An admin calls ReputationMarket::setEntryProtocolFeeBasisPoints() to set the protocol fee to 5%.
2. Alice creates a market by calling ReputationMarket::createMarket().
3. Alice calls ReputationMarket::buyVotes() to buy votes using 10 ETH for her market. She has to pay 0.5 ETH of fees which will be sent to the protocolFeeAddress.
4. ReputationMarket::graduateMarket() is called to graduate the market.
5. The graduator calls ReputationMarket::withdrawGraduatedMarketFunds() to withdraw the amount of funds stored in the marketFunds mapping for that market but the call reverts as the balance of the contract is less than the amount of funds stored in the mapping.

Impact

Each time a user buys votes, the fees paid are stored in the marketFunds mapping for that market. This causes the contract balance to differ from the amount of funds stored in the mapping. The difference continues to increase each time votes are bought for any market. As a result, the marketFunds for a specific market may remain stuck as long as new votes are being bought for another market. Consequently, the funds for that market will be locked when it graduates, and the graduator attempts to withdraw them.

Depending on the implementation of the graduation, users may still receive their ERC-20 tokens but the protocol will not be able to receive the remaining funds.

PoC

The following test should be added in rep.graduate.test.ts:

    it('should fail to withdraw graduated market funds due to fees', async () => {
      let protocolFeeAddress: string;
      protocolFeeAddress = ethers.Wallet.createRandom().address;
      await reputationMarket.connect(deployer.ADMIN).setProtocolFeeAddress(protocolFeeAddress);

      const entryFee = 500;
      await reputationMarket.connect(deployer.ADMIN).setEntryProtocolFeeBasisPoints(entryFee);
      expect(await reputationMarket.entryProtocolFeeBasisPoints()).to.equal(entryFee);

      const buyAmount = ethers.parseEther('10');
      const initialMarketFunds = await reputationMarket.marketFunds(DEFAULT.profileId);
      const protocolFeeBalanceBefore = await ethers.provider.getBalance(protocolFeeAddress);
      const contractBalanceBefore = await ethers.provider.getBalance(reputationMarket.target);

      const { fundsPaid } = await userA.buyVotes({ buyAmount });

      const contractBalanceAfter = await ethers.provider.getBalance(reputationMarket.target);
      const contractFundsReceived = contractBalanceAfter - contractBalanceBefore;
      const protocolFeeBalanceAfter = await ethers.provider.getBalance(protocolFeeAddress);
      const protocolFeeReceived = protocolFeeBalanceAfter - protocolFeeBalanceBefore;

      expect(fundsPaid).to.equal(
        contractFundsReceived + protocolFeeReceived,
        'Actual funds paid should be equal to funds received by the contract and protocol fee',
      );

      expect(protocolFeeReceived).to.equal(ethers.parseEther('0.5'));

      // contract balance difference is equal to fundsPaid - fee as protocol fee is already sent to fee address
      expect(contractFundsReceived).to.equal(fundsPaid - protocolFeeReceived);
      
      const fundsAfterBuy = await reputationMarket.marketFunds(DEFAULT.profileId);

      // but market funds stores fees aswell
      expect(fundsAfterBuy).to.equal(initialMarketFunds + contractFundsReceived + protocolFeeReceived);

      await reputationMarket.connect(graduator).graduateMarket(DEFAULT.profileId);
      
      // when the graduator tries to withdraw the market funds, it will revert
      // as market funds stores fundsPaid + protocol fee, but the protocol fee
      // has already been sent to the protocol fee address
      // hence, the balance of the contract is less than withdrawGraduatedMarketFunds() tries to send
      await expect(
        reputationMarket.connect(graduator).withdrawGraduatedMarketFunds(DEFAULT.profileId)
      ).to.be.revertedWith("ETH transfer failed");
    });

Mitigation

The fee amounts should not be stored in the marketFunds mapping.