Summary
burn() function in SmartVaultV3 does not change variable "minted" correctly, hence when user wants to leave the protocol completely, even after burning all tokens they have, they can't remove their collateral.
Vulnerability Details
In an ideal scenario (expected scenario) when users burnt all their EUROs, users should be able to leave the protocol with removing all their collateral. This scenario is respected within canRemoveCollateral() function as can be seen below:
function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
if (minted == 0) return true;
...
So minted == 0 means user burnt all their EURO's and can remove their collateral without checking further details.
But the problem arises from discrepancy between mint() and burn() functions.
While minting, mint fee is added to the "minted" variable as we can see below:
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;
...
But while burning, this fee amount does not removed back:
function burn(uint256 _amount) external ifMinted(_amount) {
uint256 fee = _amount * ISmartVaultManagerV3(manager).burnFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
minted = minted - _amount;
...
Hence minted variable does not set back to "0" in case of full burn.
We can see this in action in the following test:
Test Setup:
After npm installation,
1 - run "forge init --force" in the "2023-12-the-standard" folder. (Assuming you have git cloned the repo)
2 - Create remappings.txt in the root folder with following content:
@forge-std/=lib/forge-std/
@openzeppelin/=node_modules/@openzeppelin/
@chainlink/=node_modules/@chainlink/
3 - foundry.toml should look like this:
[profile.default]
src = 'contracts'
out = 'out'
libs = ['node_modules', 'lib']
test = 'test/foundry/'
cache_path = 'cache_forge'
4 - Inside the test folder (where hardhat tests are) create a folder "foundry" and inside that folder create a file "SmartVault.t.sol" and paste the following setup inside:
pragma solidity 0.8.17;
import {Test, console2} from "forge-std/Test.sol";
import {ERC20Mock} from "../../contracts/utils/ERC20Mock.sol";
import {EUROsMock} from "../../contracts/utils/EUROsMock.sol";
import {ChainlinkMock} from "../../contracts/utils/ChainlinkMock.sol";
import {SmartVaultManagerV5} from "../../contracts/SmartVaultManagerV5.sol";
import {SmartVaultV3} from "../../contracts/SmartVaultV3.sol";
import {PriceCalculator} from "../../contracts/utils/PriceCalculator.sol";
import {SmartVaultDeployerV3} from "../../contracts/utils/SmartVaultDeployerV3.sol";
import {TokenManagerMock} from "../../contracts/utils/TokenManagerMock.sol";
import {SmartVaultManager} from "../../contracts/utils/SmartVaultManager.sol";
import {TransparentUpgradeableProxy, ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {NFTMetadataGenerator} from "../../contracts/utils/nfts/NFTMetadataGenerator.sol";
import {SmartVaultIndex} from "../../contracts/utils/SmartVaultIndex.sol";
import {LiquidationPoolManager} from "../../contracts/LiquidationPoolManager.sol";
import {LiquidationPool} from "../../contracts/LiquidationPool.sol";
import {SwapRouterMock} from "../../contracts/utils/SwapRouterMock.sol";
import {ILiquidationPoolManager} from "../../contracts/interfaces/ILiquidationPoolManager.sol";
import {ITokenManager} from "../../contracts/interfaces/ITokenManager.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract SmartVaultV3Test is Test{
address public deployer = vm.addr(1);
address public alice = makeAddr("alice");
address public bob = makeAddr("bob");
address public john = makeAddr("john");
address public attacker = makeAddr("attacker");
address public treasury = vm.addr(4);
ERC20Mock public TST;
ERC20Mock public WBTC;
EUROsMock public EURO;
ChainlinkMock public ClEthUsd;
ChainlinkMock public ClEurUsd;
ChainlinkMock public ClWbtcUsd;
SmartVaultManagerV5 public vaultManagerV5;
SmartVaultManagerV5 public vaultManagerV5Instance;
address public vaultAddress;
SmartVaultV3 public vault;
SmartVaultDeployerV3 public vaultDeployer;
string public ETH = "ETH";
bytes32 public ethBytes32;
TokenManagerMock public tokenManager;
SmartVaultManager public vaultManagerFirstImplementation;
TransparentUpgradeableProxy public proxy;
ProxyAdmin public proxyAdmin;
SmartVaultManager public vaultManagerFirstImplementationProxiedInstance;
NFTMetadataGenerator public nftMetadataGenerator;
SmartVaultIndex public smartVaultIndex;
LiquidationPoolManager public liquidationPoolManager;
LiquidationPool public liquidationPool;
SwapRouterMock public swapRouterMock;
function stringToBytes32(string memory source) public pure returns (bytes32 result) {
bytes memory tempEmptyStringTest = bytes(source);
if (tempEmptyStringTest.length == 0) {
return 0x0;
}
assembly {
result := mload(add(source, 32))
}
}
function setUp() public {
vm.startPrank(deployer);
TST = new ERC20Mock("The Standard Token", "TST", 18);
WBTC = new ERC20Mock("Wrapped BTC", "WBTC", 8);
EURO = new EUROsMock();
skip(5 hours);
ClEthUsd = new ChainlinkMock("ETH / USD");
ClEthUsd.setPrice(237538000000);
ClEurUsd = new ChainlinkMock("EUR / USD");
ClEurUsd.setPrice(109586000);
ClWbtcUsd = new ChainlinkMock("WBTC / USD");
ClWbtcUsd.setPrice(4411586000000);
vaultManagerFirstImplementation = new SmartVaultManager();
nftMetadataGenerator = new NFTMetadataGenerator();
smartVaultIndex = new SmartVaultIndex();
ethBytes32 = stringToBytes32(ETH);
vaultDeployer = new SmartVaultDeployerV3(ethBytes32, address(ClEurUsd));
tokenManager = new TokenManagerMock(ethBytes32, address(ClEthUsd));
tokenManager.addAcceptedToken(address(WBTC), address(ClWbtcUsd));
bytes memory setUpData = abi.encodeWithSignature(
"initialize(uint256,uint256,address,address,address,address,address,address,address)",
110000,
500,
address(EURO),
address(0),
address(0),
address(tokenManager),
address(vaultDeployer),
address(smartVaultIndex),
address(nftMetadataGenerator)
);
proxyAdmin = new ProxyAdmin();
proxy = new TransparentUpgradeableProxy(address(vaultManagerFirstImplementation), address(proxyAdmin), setUpData);
vaultManagerFirstImplementationProxiedInstance = SmartVaultManager(address(proxy));
vaultManagerV5 = new SmartVaultManagerV5();
proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(proxy)), address(vaultManagerV5));
vaultManagerV5Instance = SmartVaultManagerV5(address(proxy));
liquidationPoolManager = new LiquidationPoolManager(address(TST), address(EURO), address(vaultManagerV5Instance), address(ClEurUsd), payable(treasury), 30000 );
liquidationPool = LiquidationPool(liquidationPoolManager.pool());
vaultManagerV5Instance.setProtocolAddress(address(liquidationPoolManager));
vaultManagerV5Instance.setLiquidatorAddress(address(liquidationPoolManager));
vaultManagerV5Instance = SmartVaultManagerV5(address(proxy));
smartVaultIndex.setVaultManager(address(vaultManagerV5Instance));
EURO.grantRole(EURO.DEFAULT_ADMIN_ROLE(), address(vaultManagerV5Instance));
vm.stopPrank();
}
Here comes the test function:
function testCantLeaveProtocol() public {
vm.deal(alice , 100e18);
deal(address(WBTC), alice, 10e8);
deal(address(EURO), alice, 10e18);
vm.startPrank(alice);
(address vaultTemp, ) = vaultManagerV5Instance.mint();
SmartVaultV3 vault0 = SmartVaultV3(payable(vaultTemp));
payable(vault0).call{value: 1e18}("");
IERC20(address(WBTC)).transfer(address(vault0), 1e8);
EURO.approve(address(vault0), 100_000e18);
vault0.mint(alice, 1000e18);
console2.log(EURO.balanceOf(alice));
vault0.burn(1000e18);
console2.log(EURO.balanceOf(alice));
vault0.removeCollateral(bytes32(bytes(WBTC.symbol())), 1e8, msg.sender);
}
Then run command "forge test --match-test testCantLeaveProtocol"
As we can see alice deposits some amount(unnecessary for our vulnerability), then mint 1000e18 EUROs, then burns these 1000e18 EUROs.
Then finally without any idea of what she would encounter, tries to removeCollateral that she deposited. But wait, what's happening here:
Running 1 test for test/foundry/SmartVault.t.sol:SmartVaultV3Test
[FAIL. Reason: revert: err-under-coll] testCantLeaveProtocol() (gas: 3543437)
Logs:
1010000000000000000000
5000000000000000000
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.55ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/foundry/SmartVault.t.sol:SmartVaultV3Test
[FAIL. Reason: revert: err-under-coll] testCantLeaveProtocol() (gas: 3543437)
Encountered a total of 1 failing tests, 0 tests succeeded
As you can see she can't get her collateral back because minted variable is not reset back to 0.
In order to see that the problem arises from "minted" variable I did go one step further and changed the variable "minted" in SmartVaultV3 to public and printed the result (added following line to test after minting and burning):
console2.log("minted", vault0.minted());
Here is the log afterwards:
Logs:
1010000000000000000000
minted 1005000000000000000000
5000000000000000000
minted 5000000000000000000
Impact
Users can not remove their collateral hence this is a high impact, since this will happen for every user, it is also high likelihood. Hence this is a high severity vulnerability.
Tools Used
Manual Review, Foundry Testing
Recommendations
Considering mint and burn fees are same, if we add "-fee" to the burn function's minted variable changing process, we get rid of the problem (Tried it and minted variable became 0 after burning and test didn't failed).
It should look like this:
function burn(uint256 _amount) external ifMinted(_amount) {
uint256 fee = _amount * ISmartVaultManagerV3(manager).burnFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
minted = minted - _amount -fee;
...
