Manipulating `poolId` can lead to unauthorized access to fees from a different pool, causing financial losses to the legitimate participants of that pool.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L357-L363 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L357-L371

Vulnerability details

Impact

The calculateAndSendFee function proceeds to calculate the fees and transfer them to the treasury based on the assumption that the addresses obtained from getPoolAddresses correspond to the correct pool. If the poolId is manipulated to refer to a different pool, the function will erroneously interact with the wrong tokens and potentially extract fees from the unintended pool.

If the poolId is manipulated by a malicious actor, it can lead to the following consequences:

1. An attacker can potentially extract fees from a different pool to which they do not have any right, causing financial losses for the legitimate participants of that pool.

2. Fees extracted from the wrong pool could lead to the loss of funds from the affected pool, affecting its liquidity and stability.

Proof of Concept

calculateAndSendFee function

calculateAndSendFee function #L357-L363

  function calculateAndSendFee(
    uint poolId, 
    uint token0Amount, 
    uint token1Amount, 
    address collateralAsset
  ) internal returns (uint feeAmount) {
    (, IPriceOracle oracle,, address token0, address token1) = getPoolAddresses(poolId);
      ...
   }

The function getPoolAddresses returns the addresses of token0 and token1 based on the poolId. The poolId comes from untrusted external input, making this function susceptible to potential issues if the poolId is manipulated.

Instances: Manipulation of poolId to extract fees from the wrong pool

Let's consider the following way:

1. There are two different pools with IDs poolIdA and poolIdB. Each pool contains a pair of tokens, tokenA and tokenB, with different token addresses.

2. An attacker interacts with the calculateAndSendFee function by providing an external input manipulatedPoolId, which is the ID of poolIdA.

3. The calculateAndSendFee function internally calls getPoolAddresses with the manipulatedPoolId as an argument.

4. The getPoolAddresses function is manipulated to return the addresses of tokenX and tokenY instead of tokenA and tokenB, respectively.

5. The calculateAndSendFee function now assumes that the retrieved addresses are for tokenA and tokenB and proceeds to interact with these tokens.

6. However, since the getPoolAddresses function returned the addresses of tokenX and tokenY, the calculateAndSendFee function mistakenly performs fee calculations and transfers on the wrong tokens.

7. The attacker can now potentially extract fees from poolIdA, even though they have not contributed to that pool or are not entitled to these fees.

Tools Used

VsCode

Recommended Mitigation Steps

Need to ensure that the poolId input is properly validated and that the getPoolAddresses function returns the correct token addresses corresponding to the provided poolId. This way, the contract can prevent such manipulations and protect the integrity of its fee distribution mechanism.

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as low quality report

[141345]

lack POC how getPoolAddresses is manipulated

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof