Test addresses and incorrect interface in code prevent integration with UniswapV3 and Camelot

[c4-submissions]

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L20 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/UniV3Relayer.sol#L18 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L41-L42

Vulnerability details

Proof of Concept

Testing addresses for Camelot and UniswapV3 factories are still used in the code:

CamelotRelayer.sol#L20

  address internal constant _CAMELOT_FACTORY = GOERLI_CAMELOT_V3_FACTORY;

UniV3Relayer.sol#L18

  address internal constant _UNI_V3_FACTORY = GOERLI_UNISWAP_V3_FACTORY;

Additionally, the correct interface to get the camelotPair is commented out:

CamelotRelayer.sol#L41-L42

    // camelotPair = ICamelotFactory(_CAMELOT_FACTORY).getPair(_baseToken, _quoteToken);
    camelotPair = IAlgebraFactory(_CAMELOT_FACTORY).poolByPair(_baseToken, _quoteToken);

Impact

It will prevent integration with UniswapV3 and Camelot.

Tool used

Manual Review

Recommended Mitigation Steps

Make the changes necessary to be compatible with Arbitrum One.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #119

[c4-judge]

MiloTruck marked the issue as selected for report

[MiloTruck]

While the protocol team is most likely aware of this and these addresses are probably set to testnet ones currently for testing purposes, it is an undeniable fact that if the current code was to be deployed without any modifications, the CamelotRelayer and UniV3Relayer contracts would not function as expected.

As such, I believe medium severity is appropriate.

[c4-judge]

MiloTruck marked the issue as satisfactory

[pi0neerpat]

The problem stated is that the Camelot and Uniswap factory addresses set in the Registry are set to Goerli addresses, not Mainnet. In production, we will swap Goerli addresses for Mainnet. Perhaps there is a better way to manage this than manually changing the constant variable in the Registry, however the recommendation does provide a helpful suggestion.

[mcgrathcoutinho]

Hi @MiloTruck, I believe this issue is QA/Low-severity at best since the team is clearly aware of this and has intentionally used Goerli addresses for testnet purposes. Additionally as the sponsor mentioned above In production, we will swap Goerli addresses for Mainnet further clarifies this. The only advice/value provided to the sponsor is to not hardcode the addresses and use another way to manage setting these addresses.

[MiloTruck]

Hi @mcgrathcoutinho, all findings have to judged with the assumption that the code in-scope will be deployed without modifications, unless stated otherwise by the sponsor beforehand in the contest's README.

While I do agree that the sponsor was probably aware of this, this issue is still a valid medium severity bug according to C4 rules.

[daopunk]

Okay, agreed. Although we were previously aware of this issue, we did not explicitly state that these variables would be updated before mainnet deployment, so therefore it is a valid find. Since this issue would prevent the liquidity pool relayers from functioning and prevent a price reading, it seems fair to label it medium severity.