code423n4 commented 2 years ago

Handle

leastwood

Vulnerability details

Impact

Considering that the chainlink oracles used by the Cvx3CrvOracle.sol contract are expected to be denominated in ETH, I think it would be worthwhile to add a check in setSource to ensure DAI.decimals(), USDC.decimals() and USDT.decimals() all return 18 as their result.

Proof of Concept

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/Cvx3CrvOracle.sol#L36-L50

function setSource(
    bytes32 cvx3CrvId_,
    bytes32 ethId_,
    ICurvePool threecrv_,
    AggregatorV3Interface DAI_,
    AggregatorV3Interface USDC_,
    AggregatorV3Interface USDT_
) external auth {
    cvx3CrvId = cvx3CrvId_;
    ethId = ethId_;
    threecrv = threecrv_;
    DAI = DAI_;
    USDC = USDC_;
    USDT = USDT_;
}

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/Cvx3CrvOracle.sol#L120-L127

(, int256 daiPrice, , , ) = DAI.latestRoundData();
(, int256 usdcPrice, , , ) = USDC.latestRoundData();
(, int256 usdtPrice, , , ) = USDT.latestRoundData();

require(
    daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0,
    "Chainlink pricefeed reporting 0"
);

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider implementing the suggestion above.

alcueca commented 2 years ago

Well, the Chainlink aggregators for DAI, USDC and USDT wrt ETH are 18 decimals.

The check that we are passing the right addresses should be done externally, as stated in the README.

GalloDaSballo commented 2 years ago

I think both sides have a point here. Personally my main observation is that the function using auth ultimately shows a potential single point of failure / admin privilege.

Ultimately the sponsor can check before setting the oracles address which is in line with their requirement in the readme.

I'll mark the finding as invalid although the sponsor may want to consider hardcoding the chainlink price fees to provide further security guarantees to the protocol users

code-423n4 / 2022-01-yield-findings