The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

status() function can return stale data or the L2 sequencer could be down, allowing minted > maxMintable() in reality or an incorrect distributeAssets()

t0x1c

Summary

  • distributeAssets() fetches latestRoundData from chainlink which is never checked for staleness or an incomplete round or if the L2 sequencer is down and as such can result in incorrect rewards/assets being distributed, since the protocol could be relying on outdated values

  • Similarly, the following functions all make use of the variable calculator = IPriceCalculator(_priceCalculator) present on L40 to fetch latestRoundData from chainlink. These can result in the user minting more tokens than the allowed maxMintable, since the protocol could be relying on outdated values:

  • Additionally, there's the issue of "Unhandled Chainlink Revert - Denial Of Service" as outlined in Cyfrin's article as no try/catch blocks have been used in functions like distributeAssets() while calling latestRoundData().

Vulnerability Details

  • Unchecked chainlink data being used inside distributeAssets():

function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
@---> (,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
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,,,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
uint256 _portion = asset.amount * _positionStake / stakeTotal;
....
....

  • calculator variable is used to call tokenToEurAvg(), tokenToEur() and eurToToken() which internally call latestRoundData() on L47, L56 and L63 but never check if the data is stale.

    Hence, maxMintable() can be an outdated value resulting in the user able to remove more collateral than they should be able to or even minting more than they should be able to.

  • Additionally, calls to Chainlink could revert, which may result in a complete Denial-of-Service as outlined in OZ blog. Chainlink multisigs can immediately block access to price feeds at will, so just because a price feed is working today does not mean it will continue to do so indefinitely. Smart contracts should handle this by wrapping calls to Oracles in try/catch blocks and dealing appropriately with any errors.

function euroCollateral() private view returns (uint256 euros) {
ITokenManager.Token[] memory acceptedTokens = getTokenManager().getAcceptedTokens();
for (uint256 i = 0; i < acceptedTokens.length; i++) {
ITokenManager.Token memory token = acceptedTokens[i];
@------> euros += calculator.tokenToEurAvg(token, getAssetBalance(token.symbol, token.addr));
}
}
function maxMintable() private view returns (uint256) {
@---> return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
}
function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
if (minted == 0) return true;
@---> uint256 currentMintable = maxMintable();
uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
return currentMintable >= eurValueToRemove &&
minted <= currentMintable - eurValueToRemove;
}
function mint(address _to, uint256 _amount) external onlyOwner ifNotLiquidated {
uint256 fee = _amount * ISmartVaultManagerV3(manager).mintFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
@---> require(fullyCollateralised(_amount + fee), UNDER_COLL);
minted = minted + _amount + fee;
EUROs.mint(_to, _amount);
EUROs.mint(ISmartVaultManagerV3(manager).protocol(), fee);
emit EUROsMinted(_to, _amount, fee);
}

Impact

Use of stale values (or a down L2 sequencer, or a Chainlink revert) can

  • result in breaking the invariant that the user should not be able to mint more than maxMintable.

  • result in user being able to remove more than allowed collateral.

  • result in either more or less than expected rewards/assets transferred via distributeAssets().

  • result in a complete Denial-of-Service to those calling distributeAssets().

Tools Used

Manual inspection

Recommendations

While fetching the latestRoundData in PriceCalculator.sol L47, L56 and L63, add the following checks each time. Similar check needs to be added inside distributeAssets():

- (, int256 eurUsdPrice,,,) = clEurUsd.latestRoundData();
+ (uint80 roundId, int256 eurUsdPrice,, uint256 updatedAt, uint80 answeredInRound) = clEurUsd.latestRoundData();
+ require(eurUsdPrice > minAnswer && eurUsdPrice < maxAnswer, "Chainlink price outside min & max bounds"); // @audit : protocol should set reasonable limits for `minAnswer` & `maxAnswer` beforehand
+ require(updatedAt > block.timestamp - staleTimePeriod[tokenPair][network], "Stale price"); // @audit : set `staleTimePeriod` to some value based on the network & `heartbeat` of that token-pair. See here: https://docs.chain.link/data-feeds/price-feeds/addresses/?network=arbitrum&page=1
+ require(answeredInRound >= roundId, "Stale price");

Also, we need to check if the L2 sequencer is up before fetching the above price feed. Code needs to be added as per: https://docs.chain.link/data-feeds/l2-sequencer-feeds

(
/*uint80 roundID*/,
int256 answer,
uint256 startedAt,
/*uint256 updatedAt*/,
/*uint80 answeredInRound*/
) = sequencerUptimeFeed.latestRoundData();
​
// Answer == 0: Sequencer is up
// Answer == 1: Sequencer is down
bool isSequencerUp = answer == 0;
if (!isSequencerUp) {
revert SequencerDown();
}
​
// Make sure the grace period has passed after the
// sequencer is back up.
uint256 timeSinceUp = block.timestamp - startedAt;
if (timeSinceUp <= GRACE_PERIOD_TIME) {
revert GracePeriodNotOver();
}

In addition to the above, surround the chainlink calls with try/catch as explained here.

Updates

Lead Judging Commences

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

Arbitrum-sequncer

aamirusmani1552 Auditor
over 1 year ago
dimulski Auditor
over 1 year ago
00xSEV Auditor
over 1 year ago
hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Arbitrum-sequncer

Support

FAQs

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