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

Vault can be liquidated due to wrong `undercollateralised()` return value

amarfares

Vulnerability Details

When a vault is to be liquidated by SmartVaultManagerV5.sol#liquidateVault() the return value of SmartVaultV3.sol#undercollateralised() is checked against and if true, the vault is liquidated.

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultManagerV5.sol#L81-L92

try vault.undercollateralised() returns (bool _undercollateralised) {
require(_undercollateralised, "vault-not-undercollateralised");
vault.liquidate();

The way undercollateralised() calculates whether to return true/false is by checking if minted > maxMintable():

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L99-L101

function undercollateralised() public view returns (bool) {
return minted > maxMintable();
}

Where maxMintable() takes a value from euroCollateral()

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L75-L77

function maxMintable() private view returns (uint256) {
return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
}

euroCollateral() loops through the accepted tokens list and invokes calculator.tokenToEurAvg().

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L67-L73

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));
}
}

If we take a closer look tokenToEurAvg() we can see that it returns data based of a Chainlink price feed.

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/utils/PriceCalculator.sol#L43-L49

function tokenToEurAvg(ITokenManager.Token memory _token, uint256 _tokenValue) external view returns (uint256) {
Chainlink.AggregatorV3Interface tokenUsdClFeed = Chainlink.AggregatorV3Interface(_token.clAddr);
uint256 scaledCollateral = _tokenValue * 10 ** getTokenScaleDiff(_token.symbol, _token.addr);
uint256 collateralUsd = scaledCollateral * avgPrice(4, tokenUsdClFeed);
(, int256 eurUsdPrice,,,) = clEurUsd.latestRoundData();
return collateralUsd / uint256(eurUsdPrice);
}

The issue is that the return values taken from latestRoundData() have no sanity checks whatsoever, price can be stale and inaccurate which means that tokenToEurAvg() could produce wrong results. If that happens, maxMintable() can produce wrong results and pass the check for undercollaterised() where minted > maxMintable(). Although this might be a rare instance, this would allow a vault that contains exactly 1 collateral token which returns a stale result, to be liquidated when it shouldn't be.

Another thing I want to note is that anyone can call LiquidationPoolManager.sol#runLiquidation() for a vaultId represented as tokenId. There is no access control whatsoever. This allows anyone to attempt to liquidate a vault. The runLiquidation() function invokes SmartVaultMangerV5.sol#liquidateVault() with permissions.

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPoolManager.sol#L59-L82

ISmartVaultManager manager = ISmartVaultManager(smartVaultManager);
manager.liquidateVault(_tokenId);

Although the contract PriceCalculator.sol which returns the value for tokenToEurAvg() is out of scope, the issue itself occurs because of functions invoked in the contracts in scope. I believe the issue is dangerous and upon consulting with the developer of the protocol in a private thread, they advised me to submit this due to its urgency and leave it up to the judge to decide. I have decided to submit it as Medium since it is rather unlikely but the impact is high.

Impact

Vault can be liquidate when it shouldnt be

Tools Used

Manual Review

Recommendations

Include sanity checks for getLatestData()

(uint80 roundID ,uint256 answer,, uint256 timestamp, uint80 answeredInRound) = clEurUsd.latestRoundData();
​
require(answer > 0, "Chainlink price <= 0");
require(answeredInRound >= roundID, "Stale price");
require(block.timestamp <= timestamp + stalePriceDelay, Error.STALE_PRICE);
Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Chainlink-price

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Chainlink-price

Support

FAQs

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

Give us feedback!