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

Incorrect mint() & burn() calculations reduce subsequent minting power & charge extra fee

t0x1c

Summary

Mismatch between calculation styles of minted variable inside mint() and inside burn() results in the following issues. These occur because fee is not taken into account in a consistent manner while calculating minted -

  • minted is not reset to zero even when the user burns the amount he minted. He has to pay more fee in case he wants to reset this to zero.

  • If he does not pay more fee during burn(), results in a reduced ability for the user to mint() now. He is not able to mint the max amount.

  • If he does not pay more fee during burn(), he is not able to remove all of his collateral since canRemoveCollateral() does not allow him to do so by returning false.

SmartVaultV3.sol#L160-L167

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

SmartVaultV3.sol#L169-L175

function burn(uint256 _amount) external ifMinted(_amount) {
uint256 fee = _amount * ISmartVaultManagerV3(manager).burnFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
@----> minted = minted - _amount;
EUROs.burn(msg.sender, _amount);
IERC20(address(EUROs)).safeTransferFrom(msg.sender, ISmartVaultManagerV3(manager).protocol(), fee);
emit EUROsBurned(_amount, fee);
}

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

SmartVaultV3.sol#L156-L158

function fullyCollateralised(uint256 _amount) private view returns (bool) {
@----> return minted + _amount <= maxMintable();
}

Vulnerability Details

Let us see an example:

  • Few initial suppositions first for easier calculation (you can keep the default values too if you wish to, just that the arrived figures will be a bit different):

    • Let ETH_USD_PRICE = $1

    • Let EUR_USD_PRICE = $1

    • Let COLLATERAL_RATE = 120%

    • Let mint & burn FEE_RATE = 0.5%

  • User deposits ETH as collateral.

    • This means he can receive EUROs in his account upon minting. This is because . Hence, minted value as per L163 is . With a collateralRate of , this is his max limit since

  • User goes ahead and calls mint(1e18) to receive 1 EUROs in his account. Also, since fee is added while calculating it.

  • User wants to burn all his minted EUROs. He calls burn(1e18). He is charged the burnFee of on L173. However, L171 incorrectly reduces minted to , not resetting it to zero since fee has been excluded from this calculation.

  • User is now constrained unfairly and faces the following issues -

    • He can not now mint again the value (or including fee) and his minting capacity is reduced to

      • He can't mint because fullyCollateralized() which is called internally by mint() returns false since

      • His reduced value is because and any value greater than calculates to higher than , the maxMintable().

    • He can not now remove his full collateral since canRemoveCollateral() returns false for _amount equalling

  • If he wants to regain his full powers, he needs to agree to pass an _amount figure of to the burn() function by calling burn(1.005e18) which will reset minted to , but he will have to pay more fee due to this -

    • , thus paying this as an unfair additional fee.


Add the following inside test/smartVault.js and run via npx hardhat test --grep 'incorrect burning calculation' test/smartVault.js. The tests will pass all assertions.

describe('incorrect burning calculation', async () => {
it('miscalculates collateral ratio of user after repeated burns - part 1', async () => {
expect(await Vault.undercollateralised()).to.equal(false);
const collateralPutUp = ethers.utils.parseEther('1.206');
// @audit-info : maxMintable() would be calculated as 1.005
await user.sendTransaction({to: Vault.address, value: collateralPutUp});
// @audit-info : set prices = 1 for easier calculations
await ClEthUsd.setPrice(100000000);
await ClEurUsd.setPrice(100000000);
const { maxMintable } = await Vault.status();
expect(maxMintable).to.equal(ethers.utils.parseEther('1.005'), 'maxMintable');
​
const mintedValue = ethers.utils.parseEther('1');
await Vault.connect(user).mint(user.address, mintedValue);
expect(await Vault.undercollateralised()).to.equal(false);
const { minted } = await Vault.status();
expect(minted).to.equal(ethers.utils.parseEther('1.005'), "minted");
// approve transfer to protocol
await EUROs.connect(user).approve(Vault.address, ethers.utils.parseEther('1000'));
​
// ****************************************** INTERMEDIATE SETUP STEPS *****************************************************
// ******** let's use another account to credit `user` with some EUROs which can be used to pay burn-fee later *************
await VaultManager.connect(otherUser).mint();
const { status } = (await VaultManager.connect(otherUser).vaults())[0];
const { vaultAddress } = status;
const Vault2 = await ethers.getContractAt('SmartVaultV3', vaultAddress);
await otherUser.sendTransaction({to: Vault2.address, value: ethers.utils.parseEther('99')});
await Vault2.connect(otherUser).mint(user.address, ethers.utils.parseEther('50'));
​
// @audit-info : `user` should now have 51 EUROs => 50 EUROs given by `otherUser` + 1 EUROs minted by `user` himself.
expect(await EUROs.balanceOf(user.address)).to.equal(ethers.utils.parseEther('51'));
// ********************************* EUROs for burn-fee credited to `user` now *********************************************
// @audit-info : `user` wants to burn all minted EUROs
const burnedValue = ethers.utils.parseEther('1');
burn = Vault.connect(user).burn(burnedValue);
await expect(burn).to.emit(Vault, "EUROsBurned").withArgs(burnedValue, ethers.utils.parseEther('0.005'));
let [, updatedMinted,,,,,,] = await Vault.status();
// @audit-info : Bug. `minted` should have been equal to 0 here, but it's 0.005 now, since fee was not taken into account.
expect(updatedMinted).to.equal(ethers.utils.parseEther('0.005'), "afterBurn");
​
// @audit-info : capacity of `user` to mint decreases progressively each time he mints, eventually reaching 0. Next time, he
// will be able to mint only `(maxMintable() - minted - someFee) => 0.9950....`.
let notAllowedMintableAmount = '.995024875621890549'; // @audit : this is less than the value of 1 which one would expect
let allowedMintableAmount = '.995024875621890548';
await expect(Vault.connect(user).mint(user.address, ethers.utils.parseEther(notAllowedMintableAmount))).to.be.revertedWith('err-under-coll');
await expect(Vault.connect(user).mint(user.address, ethers.utils.parseEther(allowedMintableAmount))).to.not.be.reverted;
// @audit-info : This also means that in order to burn everything in the above example, he should have burnt
// `1.005` instead of `1`, effectively paying `0.005 * 1.005 = 0.005025` as fee instead of the expected `0.005`.
// Check that out in the following test: `miscalculates collateral ratio of user after repeated burns - part 2`.
​
});
​
it('miscalculates collateral ratio of user after repeated burns - part 2', async () => {
expect(await Vault.undercollateralised()).to.equal(false);
const collateralPutUp = ethers.utils.parseEther('1.206');
// @audit-info : maxMintable() would be calculated as 1.005
await user.sendTransaction({to: Vault.address, value: collateralPutUp});
// @audit-info : set prices = 1 for easier calculations
await ClEthUsd.setPrice(100000000);
await ClEurUsd.setPrice(100000000);
const { maxMintable } = await Vault.status();
expect(maxMintable).to.equal(ethers.utils.parseEther('1.005'), 'maxMintable');
​
const mintedValue = ethers.utils.parseEther('1');
await Vault.connect(user).mint(user.address, mintedValue);
expect(await Vault.undercollateralised()).to.equal(false);
const { minted } = await Vault.status();
expect(minted).to.equal(ethers.utils.parseEther('1.005'), "minted");
// approve transfer to protocol
await EUROs.connect(user).approve(Vault.address, ethers.utils.parseEther('1000'));
​
// ****************************************** INTERMEDIATE SETUP STEPS *****************************************************
// ******** let's use another account to credit `user` with some EUROs which can be used to pay burn-fee later *************
await VaultManager.connect(otherUser).mint();
const { status } = (await VaultManager.connect(otherUser).vaults())[0];
const { vaultAddress } = status;
const Vault2 = await ethers.getContractAt('SmartVaultV3', vaultAddress);
await otherUser.sendTransaction({to: Vault2.address, value: ethers.utils.parseEther('99')});
await Vault2.connect(otherUser).mint(user.address, ethers.utils.parseEther('50'));
​
// @audit-info : `user` should now have 51 EUROs => 50 EUROs given by `otherUser` + 1 EUROs minted by `user` himself.
expect(await EUROs.balanceOf(user.address)).to.equal(ethers.utils.parseEther('51'));
// ********************************* EUROs for burn-fee credited to `user` now *********************************************
// @audit-info : `user` wants to burn all minted EUROs. But this time, he passes `amount + fairBurnFee` as param to the burn function.
const burnedValue = ethers.utils.parseEther('1.005'); // amount + fairBurnFee
burn = Vault.connect(user).burn(burnedValue);
// @audit-info : Fee charged on top of this = 0.005 * 1.005 = 0.005025
await expect(burn).to.emit(Vault, "EUROsBurned").withArgs(burnedValue, ethers.utils.parseEther('0.005025')); // paid more fee
let [, updatedMinted2,,,,,,] = await Vault.status();
// @audit-info : All burned now.
expect(updatedMinted2).to.equal(ethers.utils.parseEther('0'), "afterBurningEverything");
});
});

Impact

  • User is forced to pay extra fee otherwise his capability to call other functions like removeCollateral() & mint() is unfairly limited.

  • There is also some degree of risk that if the user does not notice the ever increasing value of minted after each mint() & burn() cycle of exact same amounts, then he may eventually get under-collateralized and be liquidated.

Tools Used

Hardhat

Recommendations

Multiple approaches can be taken to correct this, one of which could be:

- function burn(uint256 _amount) external ifMinted(_amount) {
+ function burn(uint256 _amount) external {
uint256 fee = _amount * ISmartVaultManagerV3(manager).burnFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
+ require(minted >= _amount + fee, "err-insuff-minted");
- minted = minted - _amount;
+ minted = minted - _amount - fee;
EUROs.burn(msg.sender, _amount);
IERC20(address(EUROs)).safeTransferFrom(msg.sender, ISmartVaultManagerV3(manager).protocol(), fee);
emit EUROsBurned(_amount, fee);
}

Important Considerations

Note that the in the above approach, mintFeeRate and burnFeeRate need to be equal -

  • If mintFeeRate > burnFeeRate, then the same problem occurs as above despite the fix, albeit at a slower pace.

  • If mintFeeRate < burnFeeRate, then the above fix's require statement will revert and the user won't be able to burn all of his minted amount.

There are other ways to handle the scenario if the protocol wishes to have the liberty of setting mintFeeRate & burnFeeRate as different numbers. You may then exclude fee from the minted calculation inside the mint() function but it has repurcussions across the protocol, such as while liquidating an underwater vault. These will have to be further examined.

Constraining mintFeeRate & burnFeeRate to be equal seems to be the simpler solution.

Updates

Lead Judging Commences

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

fee-loss

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

fee-loss

Support

FAQs

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