Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

The performance (APR) of GMXVault would be declined due to lack of a logic to switch the Oracle when the ChainlinkOracle contract would be paused

0xmuxyz

Summary

Once the ChainlinkOracle contract would be paused, calling the GMXVault#compound() by the Keeper would always fail (until the ChainlinkOracle contract would be unpaused). Because there is no logic to switch the Oracle.
This (keeping failing to compound earned-yield) lead to that the performance (APR) of GMXVault would be declined.

Vulnerability Details

The ChainlinkOracle contract could be paused by the owner like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkOracle.sol#L11
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkOracle.sol#L241-L243
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkOracle.sol#L248-L250

contract ChainlinkOracle is Ownable2Step, Pausable { ///<--------- @audit
...
/**
* @notice Emergency pause of this oracle
*/
function emergencyPause() external onlyOwner whenNotPaused { ///<--------- @audit
_pause();
}
​
/**
* @notice Emergency resume of this oracle
*/
function emergencyResume() external onlyOwner whenPaused { ///<--------- @audit
_unpause();
}

When compounding earned yield for a GMXVault (Strategy Vault), the GMXCompound#compound() would be called via the GMXVault#compound() by the Keeper.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L502

Within the GMXCompound#compound(), tokenA and tokenB would be swapped via the GMXManager#swapExactTokensForTokens() before adding liquidity to the GM Pool via the GMXManager#addLiquidity() like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXCompound.sol#L72

/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function compound(
GMXTypes.Store storage self,
GMXTypes.CompoundParams memory cp
) external {
...
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
...
ISwap.SwapParams memory _sp;
​
_sp.tokenIn = cp.tokenIn;
_sp.tokenOut = cp.tokenOut;
_sp.amountIn = _tokenInAmt;
_sp.amountOut = 0; // amount out minimum calculated in Swap
_sp.slippage = self.minSlippage;
_sp.deadline = cp.deadline;
​
GMXManager.swapExactTokensForTokens(self, _sp); ///<---------- @audit
...
self.compoundCache.depositKey = GMXManager.addLiquidity( ///<---------- @audit
self,
_alp
);
}

Within the GMXManager#swapExactTokensForTokens(), the GMXWorker#swapExactTokensForTokens() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXManager.sol#L294

/**
* @notice Swap exact amount of tokenIn for as many possible amount of tokenOut
* @param self GMXTypes.Store
* @param sp ISwap.SwapParams
* @return amountOut in token decimals
*/
function swapExactTokensForTokens(
GMXTypes.Store storage self,
ISwap.SwapParams memory sp
) external returns (uint256) {
if (sp.amountIn > 0) {
return GMXWorker.swapExactTokensForTokens(self, sp); ///<---------- @audit
} else {
return 0;
}
}

Within the GMXWorker#swapExactTokensForTokens(), the UniswapSwap#swapExactTokensForTokens() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWorker.sol#L120
(NOTE：The UniswapSwap contract would be used as a SwapRouter here)

/**
* @dev Swap exact amount of tokenIn for as many amount of tokenOut
* @param self Vault store data
* @param sp ISwap.SwapParams
* @return amountOut Amount of tokens out in token decimals
*/
function swapExactTokensForTokens(
GMXTypes.Store storage self,
ISwap.SwapParams memory sp
) external returns (uint256) {
IERC20(sp.tokenIn).approve(address(self.swapRouter), sp.amountIn);
​
return self.swapRouter.swapExactTokensForTokens(sp); ///<-------------- @audit
}

Within the UniswapSwap#swapExactTokensForTokens(), the ChainlinkOracle#consultIn18Decimals() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/swaps/UniswapSwap.sol#L69

/**
* @notice Swap exact amount of tokenIn for as many amount of tokenOut
* @param sp ISwap.SwapParams
* @return amountOut Amount of tokens out; in token decimals
*/
function swapExactTokensForTokens(ISwap.SwapParams memory sp) external returns (uint256) {
IERC20(sp.tokenIn).safeTransferFrom(msg.sender, address(this), sp.amountIn);
​
IERC20(sp.tokenIn).approve(address(router), sp.amountIn);
​
uint256 _valueIn = sp.amountIn * oracle.consultIn18Decimals(sp.tokenIn) / SAFE_MULTIPLIER; /// @audit info - Adjust DP
​
uint256 _amountOutMinimum = _valueIn
* SAFE_MULTIPLIER
/ oracle.consultIn18Decimals(sp.tokenOut) ///<---------------- @audit
/ (10 ** (18 - IERC20Metadata(sp.tokenOut).decimals()))
* (10000 - sp.slippage) / 10000;
...

Within the ChainlinkOracle#consultIn18Decimals(), the price of token (answer), which is normalized as 1e18, would be retrieved from the Chainlink PriceFeed Oracle. And then, it would be returned like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkOracle.sol#L69

/**
* @notice Get token price from Chainlink feed returned in 1e18
* @param token Token address
* @return price in 1e18
*/
function consultIn18Decimals(address token) external view whenNotPaused returns (uint256) {
(int256 _answer, uint8 _decimals) = consult(token); ///<---------------- @audit
​
return _answer.toUint256() * 1e18 / (10 ** _decimals);
}

For the moment, only Chainlink PriceFeed Oracle would be implemented to retrieve a spot price of tokenA and tokenB respectively.
If the ChainlinkOracle contract would be paused, the price of tokenA and tokenB can not be retrieved when the ChainlinkOracle#consultIn18Decimals() would be called via the GMXVault#compound() by the Keeper due to that calling the ChainlinkOracle#consultIn18Decimals() in the UniswapSwap# swapExactTokensForTokens() would be reverted.

To avoid it, a logic is supposed to be implemented to switch from the ChainlinkOracle contract to another alternative Oracle contract (i.e. Uniswap's TWAP Oracle-based contract).
However, within the UniswapSwap#swapExactTokensForTokens(), there is no implementation that switch from the ChainlinkOracle contract to another alternative Oracle contract (i.e. Uniswap's TWAP Oracle-based contract).
This is problematic because once the ChainlinkOracle contract would be paused, calling the GMXVault#compound() by the Keeper would keep failing for a while (until the ChainlinkOracle contract is unpaused). This lead to declining the performance (APR) of GMXVault. It would be a bad situation for the depositors of GMXVault.

Impact

Due to lack of implementation to switch other alternative Oracles (i.e. Uniswap's TWAP Oracle) besides the Chainlink PriceFeed Oracle, the performance (APR) of GMXVault would be declined during the ChainlinkOracle contract would be paused.

Tools Used

  • Foundry

Recommendations

Consider implementing a logic that switch from the ChainlinkOracle contract to another alternative Oracle contract (i.e. Uniswap's TWAP Oracle-based contract) so that retrieving price of tokenA and tokenB can keep working and therefore the compounding process via the GMXVault#compound() can keep working even if the ChainlinkOracle contract would be paused.

Updates

Lead Judging Commences

hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Chainlink oracle revert is not handled, need a backup oracle

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.