Summary
distributeAssets()does not check if the Chainlink data is stale. Arbitrum sequencer being inactive can cause stale data.
Vulnerability Details
distributeAssets() in LiquidationPool.sol checks for Chainlink price feeds in line 207 and 218.
function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
if (asset.amount > 0) {
(,int256 assetPriceUsd,,,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
However, there is no check for stale data. This could result in incorrect price feeds. It is also advisable to check if Arbitrum sequencer is active since sequencer being down can show stale data as fresh.
Cyfrin article on stale data: https://medium.com/cyfrin/chainlink-oracle-defi-attacks-93b6cb6541bf#99af
Example of this vulnerability being reported: https://solodit.xyz/issues/chainlink-price-is-used-without-checking-validity-hans-none-meta-markdown_
Impact
Stale data can cause a loss of funds and/or wrong calculations in distributeAssets() and runLiquidation() in LiquidationPoolManager.sol that calls distributeAssets() .
Tools Used
Manual review.
Recommendations
Implement a new function and constants in LiquidationPool.sol to check if Arbitrum sequencer is active. Consider changing these constants into storage variables and make them upgradable with a function, in case Chainlink changes the heartbeat of the price feeds.
uint256 public constant TIMEOUT = 2 hours;
uint256 public constant TIMEOUT_TWO = 3 hours;
uint256 public constant GRACE_PERIOD_TIME = 1 hours;
function isSequencerActive() internal view returns (bool) {
(, int256 answer, uint256 startedAt,,) = sequencer.latestRoundData();
if (block.timestamp - startedAt <= GRACE_PERIOD_TIME || answer == 1)
return false;
return true;
}
Update distributeAssets() as shown below.
function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd, ,uint256 updatedAt,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
+ uint256 secondsSince = block.timestamp - updatedAt;
+ require(isSequencerActive(), "Sequencer is down");
+ require(secondsSince < TIMEOUT, "Stale data");
uint256 stakeTotal = getStakeTotal();
uint256 burnEuros;
uint256 nativePurchased;
for (uint256 j = 0; j < holders.length; j++) {
Position memory _position = positions[holders[j]];
uint256 _positionStake = stake(_position);
if (_positionStake > 0) {
for (uint256 i = 0; i < _assets.length; i++) {
ILiquidationPoolManager.Asset memory asset = _assets[i];
if (asset.amount > 0) {
(, int256 assetPriceUsd, , uint256 updatedAt,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
+ require(secondsSince < TIMEOUT_TWO, "Stale data");
uint256 _portion = asset.amount * _positionStake / stakeTotal;
uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
* _hundredPC / _collateralRate;
if (costInEuros > _position.EUROs) {
_portion = _portion * _position.EUROs / costInEuros;
costInEuros = _position.EUROs;
}
_position.EUROs -= costInEuros;
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
burnEuros += costInEuros;
if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}
}
}
}
positions[holders[j]] = _position;
}
if (burnEuros > 0) IEUROs(EUROs).burn(address(this), burnEuros);
returnUnpurchasedNative(_assets, nativePurchased);
}
}
