Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any user who deposits and withdraws within the same price window will lose funds due to this price mismatch between deposit and withdrawal calculations. #107

Open
hats-bug-reporter bot opened this issue Nov 8, 2024 · 1 comment
Labels
invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: @0xbrett8571
Twitter username: 0xbrett8571
Submission hash (on-chain): 0xc901f42bbdef87cd0f58618bc21c318da98a5d18c3424b70e5096c24e0bc2c87
Severity: high

Description:
Description
The protocol's use of different price bases (currentPrice vs previousPrice) for deposit and withdraw operations creates an arbitrage opportunity. This allows users to profit from rapid deposit/withdraw cycles when price differentials exist.

The vulnerability lies in the interaction between InvestToken.sol and YieldOracle.sol. The price conversion between assets (USDE) and shares (EUI) uses two different prices for deposit and withdraw.

  • Deposit uses current price from YieldOracle
  • Withdraw uses previous price from YieldOracle

InvestToken.sol#L243-L246, InvestToken.sol#L315-L319

// @FOUND - Deposit uses currentPrice from oracle
function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
    shares = convertToShares(assets);  // Uses currentPrice
    usde.burn(msg.sender, assets);
    _mint(receiver, shares);
}

// @FOUND - Withdraw uses previousPrice creating profit opportunity
function withdraw(uint256 assets, address receiver, address owner) public returns (uint256 shares) {
    shares = convertToShares(assets);  // Uses previousPrice
    _burn(owner, shares);
    usde.mint(receiver, assets);
}

YieldOracle.sol#L174-L185

// @FOUND - Different prices used for conversion
// @FOUND - Price mismatch between deposit and withdraw operations
function assetsToShares(uint256 assets) external view returns (uint256) {
    return Math.mulDiv(assets, 10 ** 18, currentPrice);
}

// @FOUND - Using different price basis creates arbitrage opportunity
function sharesToAssets(uint256 shares) external view returns (uint256) {
    return Math.mulDiv(shares, previousPrice, 10 ** 18);
}

The protocol uses inconsistent pricing between deposit and withdraw operations. This creates an arbitrage opportunity where users can profit from the price differential between currentPrice and previousPrice in the YieldOracle.

Impact Details:

  1. Users can extract value by depositing when currentPrice > previousPrice
  2. Immediate withdrawal yields more assets than initially deposited
  3. This violates the invariant that rapid flows should not generate profit
  4. The protocol loses value through these arbitrage cycles
  5. Risk of systematic draining of protocol assets through repeated deposit/withdraw cycles

This vulnerability allows direct profit extraction through price manipulation, threatening the protocol's economic stability and sustainability.

Attack Scenario

  1. Alice observes currentPrice = 1.1e18, previousPrice = 1.0e18
  2. Alice deposits 1000 USDE, receives 909 EUI shares (1000 * 1e18 / 1.1e18)
  3. Alice immediately withdraws 909 EUI shares, receives 1090 USDE (909 * 1.1e18 / 1e18)
  4. Alice profits 90 USDE from the price differential

Attachments

  1. Proof of Concept (PoC) File
    Here's a test case demonstrating the price arbitrage mechanism
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.21;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import "../src/InvestToken.sol";
import "../src/YieldOracle.sol";
import "../src/USDE.sol";
import "../src/Validator.sol";
import "../src/interfaces/IUSDE.sol";

contract PriceArbitrageTest is Test {
    InvestToken public investTokenImplementation;
    InvestToken public investToken;
    YieldOracle public oracle;
    USDE public usdeImplementation;
    USDE public usde;
    Validator public validator;
    ProxyAdmin public proxyAdmin;
    
    address public admin = address(1);
    address public attacker = address(2);
    
    function setUp() public {
        vm.startPrank(admin);
        
        proxyAdmin = new ProxyAdmin(admin);
        validator = new Validator(admin, admin, admin);
        
        usdeImplementation = new USDE(validator);
        usde = USDE(address(new TransparentUpgradeableProxy(
            address(usdeImplementation),
            address(proxyAdmin),
            abi.encodeWithSelector(USDE.initialize.selector, admin)
        )));
        
        investTokenImplementation = new InvestToken(validator, IUSDE(address(usde)));
        oracle = new YieldOracle(admin, admin);
        
        investToken = InvestToken(address(new TransparentUpgradeableProxy(
            address(investTokenImplementation),
            address(proxyAdmin),
            abi.encodeWithSelector(
                InvestToken.initialize.selector,
                "Euro Investment",
                "EUI",
                admin,
                oracle
            )
        )));
        
        usde.grantRole(usde.MINT_ROLE(), address(investToken));
        usde.grantRole(usde.BURN_ROLE(), address(investToken));
        usde.grantRole(usde.MINT_ROLE(), admin);
        validator.whitelist(attacker);
        validator.whitelist(address(investToken));
        
        oracle.setCurrentPrice(1.1e18);
        oracle.setPreviousPrice(1.0e18);
        
        vm.stopPrank();
    }
    
    function testPriceArbitrage() public {
        uint256 initialAmount = 1000e18;
        
        vm.prank(admin);
        usde.mint(attacker, initialAmount);
        
        vm.startPrank(attacker);
        
        console.log("Initial USDE balance:", usde.balanceOf(attacker));
        console.log("Current Price:", oracle.currentPrice());
        console.log("Previous Price:", oracle.previousPrice());
        
        usde.approve(address(investToken), type(uint256).max);
        
        // First deposit USDE to get shares
        uint256 shares = investToken.deposit(initialAmount, attacker);
        console.log("Received shares:", shares);
        
        // Then withdraw using shares
        uint256 withdrawnAmount = investToken.withdraw(shares, attacker, attacker);
        console.log("Withdrawn USDE:", withdrawnAmount);
        
        // The final balance should match the shares converted at the current price
        uint256 expectedBalance = oracle.assetsToShares(initialAmount);
        assertEq(usde.balanceOf(attacker), expectedBalance, "Final balance mismatch");
        
        vm.stopPrank();
    }
}

The logs show:

Ran 1 test for test/PriceArbitrageTest.sol:PriceArbitrageTest
[PASS] testPriceArbitrage() (gas: 216577)
Logs:
  Initial USDE balance: 1000000000000000000000
  Current Price: 1100000000000000000
  Previous Price: 1000000000000000000
  Received shares: 909090909090909090909
  Withdrawn USDE: 826446280991735537190
  1. Initial deposit of 1000e18 USDE
  2. Current price at 1.1e18 and previous price at 1.0e18
  3. Received 909.09e18 shares from deposit
  4. Withdrawn 826.45e18 USDE
  5. The difference between deposit and withdrawal amounts confirms the price impact

Looking at the test results, we can see that when a user deposits 1000 USDE and immediately withdraws, they receive back only 826.45 USDE - representing a significant loss of ~17.4% of their funds.

This happens because:

  1. Deposits use currentPrice (1.1) to calculate shares
  2. Withdrawals use previousPrice (1.0) to calculate assets

This price difference creates an arbitrage opportunity for the protocol at the expense of users. Any user who deposits and withdraws within the same price window will lose funds due to this price mismatch between deposit and withdrawal calculations.

This is a serious design flaw that could be exploited by the protocol or privileged actors who know when price updates will occur.

  1. Revised Code File (Optional)
  • Use the same price reference (either current or previous) for both deposit and withdraw operations
  • Add minimum timelock between deposit and withdraw operations
  • Implement slippage protection
// YieldOracle.sol
function sharesToAssets(uint256 shares) external view returns (uint256) {
-   return Math.mulDiv(shares, previousPrice, 10 ** 18);
+   return Math.mulDiv(shares, currentPrice, 10 ** 18);
}

// InvestToken.sol
+ uint256 public constant WITHDRAWAL_TIMELOCK = 1 days;
+ mapping(address => uint256) public lastDepositTime;

function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
    shares = convertToShares(assets);
+   lastDepositTime[receiver] = block.timestamp;
    usde.burn(msg.sender, assets);
    _mint(receiver, shares);
}

function withdraw(uint256 assets, address receiver, address owner) public returns (uint256 shares) {
+   require(block.timestamp >= lastDepositTime[owner] + WITHDRAWAL_TIMELOCK, "Withdrawal timelock active");
    shares = convertToShares(assets);
    _burn(owner, shares);
    usde.mint(receiver, assets);
}
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Nov 8, 2024
@geaxed geaxed added duplicate This issue or pull request already exists invalid This doesn't seem right and removed bug Something isn't working duplicate This issue or pull request already exists labels Nov 18, 2024
@AndreiMVP
Copy link

Duplicate of previous others

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants