Submission Details
Severity:
Incorrect formula in `canRemoveCollateral` won't allow to withdraw some unused collateral
Summary
eurValueToRemove isn't scaled using collateralRate => doesn't allow to withdraw all the collateral. currentMintable = eurValue/collateralRate, but eurValueToRemove just eurValue
Vulnerability Details
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L127-L133
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;
}
Simple example:
collateralRate = 200% (for clarity)
maxMintable = currentMintable= €100,
collateral = €200
We minted €40. Used €80 collateral.
€120 collateral are not used, it should be allowed to withdraw it
But if we ask to withdraw even ~101, then
currentMintable >= eurValueToRemovewill be false (Because100 >= 101is false)If we ask to withdraw 90 =>
minted <= currentMintable - eurValueToRemovewill be false (Because40 <= 100 - 90=>40 <= 10is false)Allowed only <60
See PoC more examples
Impact
User is unable to withdraw part of unused collateral
Proof of Concept
You can put it in tests and run npx hardhat test test/fileName.js
// @ts-check
const { ethers } = require('hardhat');
const { BigNumber } = ethers;
const { expect } = require('chai');
const { DEFAULT_ETH_USD_PRICE, DEFAULT_EUR_USD_PRICE, DEFAULT_COLLATERAL_RATE, PROTOCOL_FEE_RATE, getCollateralOf, ETH, getNFTMetadataContract, fullyUpgradedSmartVaultManager } = require('./common');
const { HUNDRED_PC } = require('./common');
let VaultManager, Vault, TokenManager, ClEthUsd, EUROs, SwapRouterMock, MockWeth, admin, user, otherUser, protocol;
describe('SmartVault', async () => {
beforeEach(async () => {
[ admin, user, otherUser, protocol ] = await ethers.getSigners();
ClEthUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('ETH / USD');
await ClEthUsd.setPrice(DEFAULT_ETH_USD_PRICE);
const ClEurUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('EUR / USD');
await ClEurUsd.setPrice(DEFAULT_EUR_USD_PRICE);
EUROs = await (await ethers.getContractFactory('EUROsMock')).deploy();
TokenManager = await (await ethers.getContractFactory('TokenManagerMock')).deploy(ETH, ClEthUsd.address);
const SmartVaultDeployer = await (await ethers.getContractFactory('SmartVaultDeployerV3')).deploy(ETH, ClEurUsd.address);
const SmartVaultIndex = await (await ethers.getContractFactory('SmartVaultIndex')).deploy();
const NFTMetadataGenerator = await (await getNFTMetadataContract()).deploy();
SwapRouterMock = await (await ethers.getContractFactory('SwapRouterMock')).deploy();
MockWeth = await (await ethers.getContractFactory('WETHMock')).deploy();
VaultManager = await fullyUpgradedSmartVaultManager(
BigNumber.from(200000), // 200%
PROTOCOL_FEE_RATE, EUROs.address, protocol.address,
protocol.address, TokenManager.address, SmartVaultDeployer.address,
SmartVaultIndex.address, NFTMetadataGenerator.address, MockWeth.address,
SwapRouterMock.address
);
await SmartVaultIndex.setVaultManager(VaultManager.address);
await EUROs.grantRole(await EUROs.DEFAULT_ADMIN_ROLE(), VaultManager.address);
await VaultManager.connect(user).mint();
const { status } = (await VaultManager.connect(user).vaults())[0];
const { vaultAddress } = status;
Vault = await ethers.getContractAt('SmartVaultV3', vaultAddress);
});
describe('adding collateral', async () => {
it('accepts native currency as collateral', async () => {
// Note: collateralRate is set to 200% for clarity in `beforeEach`
const hundredEur = BigNumber.from(10).pow(20); //1e20 = €100 * 1e18 decimals;
// Note: value is set to ~200 Euro in eth, ~0.1325 Eth
const value = hundredEur.mul(2).mul(DEFAULT_EUR_USD_PRICE).div(DEFAULT_ETH_USD_PRICE);
await user.sendTransaction({to: Vault.address, value});
const statusBeforeMint = await parseStatus();
console.log('Nothing minted yet:');
await printStatus(statusBeforeMint);
/* Example 1: borrow 1wei, try to remove 50% of collateral */
await Vault.connect(user).mint(
user.address,
BigNumber.from(1)
);
const statusAfterMint1Wei = await parseStatus();
console.log('After 1 wei minted:');
await printStatus(statusAfterMint1Wei);
// let's try to withdraw half of our collateral,
// we minted only 1wei, it should not be a problem?
const remove = Vault.connect(user).removeCollateralNative(
value.div(2),
user.address,
);
// it reverts
await expect(remove).to.be.revertedWith('err-under-coll');
/* Example 2: borrow ~40% of allowed, try to remove 30% of collateral */
await Vault.connect(user).mint(
user.address,
hundredEur.mul(4).div(10)
);
const statusAfter40Percent = await parseStatus();
console.log('After ~40% minted:');
await printStatus(statusAfter40Percent);
const remove30Percent = Vault.connect(user).removeCollateralNative(
value.mul(30).div(100),
user.address,
);
await expect(remove30Percent).to.be.revertedWith('err-under-coll');
});
});
});
async function parseStatus() {
const { minted, maxMintable, totalCollateralValue } = await Vault.status();
const totalCollateralUsed = totalCollateralValue.mul(minted).div(maxMintable);
const totalCollateralUnused = totalCollateralValue.sub(totalCollateralUsed);
const PERCENT_MULTIPLIER = 1000;
const unusedInPercents = totalCollateralUnused
.mul(PERCENT_MULTIPLIER)
.div(totalCollateralValue)
.toNumber() / PERCENT_MULTIPLIER;
return { minted, maxMintable, totalCollateralValue, totalCollateralUsed, totalCollateralUnused, unusedInPercents };
}
async function printStatus({
minted, maxMintable, totalCollateralValue, totalCollateralUsed, totalCollateralUnused, unusedInPercents
}) {
console.log( {
minted: ethers.utils.formatEther(minted),
maxMintable: ethers.utils.formatEther(maxMintable),
totalCollateralValue: ethers.utils.formatEther(totalCollateralValue),
totalCollateralUsed: ethers.utils.formatEther(totalCollateralUsed),
totalCollateralUnused: ethers.utils.formatEther(totalCollateralUnused),
unusedInPercents: `${unusedInPercents*100}%`,
} );
}
Tools Used
Manual review
Recommended Mitigation Steps
Rewrite the formula to use collateralRate
Updates
Lead Judging Commences
hrishibhat over 1 year ago
Submission Judgement Published Assigned finding tags:
canRemoveCollateral
Prize pool breakdown
Total prize
20,000 USDC
nSLOC
60933 USDC / LOC
17,500 USDC
2,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.