Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

minDepositAmount can be set to more than maxDepositAmount

ashishlach

Summary

When setting the values in "minDepositAmount" and "maxDepositAmount" we dont check properly whether they are given correctly or not. This potentially allows updating values in reverse order as well.

This needs to be fixed in 2 places - initialize() function and setMinMaxDepositAmount() function in PerpetualVault.sol

https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L200

https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L717

Vulnerability Details

POC File: PerpetualVaultTestASH.t.sol

forge test --mp test/PerpetualVaultTestASH.t.sol --via-ir --rpc-url arbitrum -vv
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.4;
​
import {Test, console} from "forge-std/Test.sol";
import "forge-std/StdCheats.sol";
import {ArbitrumTest} from "./utils/ArbitrumTest.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {PerpetualVault} from "../contracts/PerpetualVault.sol";
import {GmxProxy} from "../contracts/GmxProxy.sol";
import {VaultReader} from "../contracts/VaultReader.sol";
import {KeeperProxy} from "../contracts/KeeperProxy.sol";
import {MarketPrices, PriceProps} from "../contracts/libraries/StructData.sol";
import {MockData} from "./mock/MockData.sol";
import {Error} from "../contracts/libraries/Error.sol";
​
interface IExchangeRouter {
struct SimulatePricesParams {
address[] primaryTokens;
PriceProps[] primaryPrices;
uint256 minTimestamp;
uint256 maxTimestamp;
}
function simulateExecuteOrder(
bytes32 key,
SimulatePricesParams memory oracleParams
) external;
}
​
interface IOrderHandler {
function executeOrder(
bytes32 key,
MockData.OracleSetPriceParams calldata oracleParams
) external;
}
​
interface IGmxUtils {
function setSlippage(uint256 _slippage) external;
}
​
/// @title PerpetualVaultTest
/// @notice Test suite for the PerpetualVault contract
/// @dev Uses Forge's standard library for testing Ethereum smart contracts
​
//@audit compile and run this forge test --mp test/PerpetualVaultTestASH.t.sol --via-ir --rpc-url arbitrum -vv
​
contract PerpetualVaultTestASH is Test, ArbitrumTest {
enum PROTOCOL {
DEX,
GMX
}
​
address payable vault;
address payable vault2x;
VaultReader reader;
MockData mockData;
​
event GmxPositionCallbackCalled(bytes32 requestKey, bool success);
​
/// @notice Sets up the test environment
/// @dev Deploys necessary contracts and initializes test data
function setUp() public {
address ethUsdcMarket = address(
0x70d95587d40A2caf56bd97485aB3Eec10Bee6336
);
address orderHandler = address(
0xe68CAAACdf6439628DFD2fe624847602991A31eB
);
// old address orderHandler = address(0xB0Fc2a48b873da40e7bc25658e5E6137616AC2Ee);
address liquidationHandler = address(
0xdAb9bA9e3a301CCb353f18B4C8542BA2149E4010
);
// old address liquidationHandler = address(0x08A902113F7F41a8658eBB1175f9c847bf4fB9D8);
address adlHandler = address(
0x9242FbED25700e82aE26ae319BCf68E9C508451c
);
// old address adlHandler = address(0x26BC03c944A4800299B4bdfB5EdCE314dD497511);
address gExchangeRouter = address(
0x900173A66dbD345006C51fA35fA3aB760FcD843b
);
// old address gExchangeRouter = address(0x69C527fC77291722b52649E45c838e41be8Bf5d5);
address gmxRouter = address(0x7452c558d45f8afC8c83dAe62C3f8A5BE19c71f6);
address dataStore = address(0xFD70de6b91282D8017aA4E741e9Ae325CAb992d8);
address orderVault = address(
0x31eF83a530Fde1B38EE9A18093A333D8Bbbc40D5
);
address gmxReader = address(0x0537C767cDAC0726c76Bb89e92904fe28fd02fE1);
// address gmxReader = address(0x5Ca84c34a381434786738735265b9f3FD814b824);
address referralStorage = address(
0xe6fab3F0c7199b0d34d7FbE83394fc0e0D06e99d
);
​
ProxyAdmin proxyAdmin = new ProxyAdmin();
​
GmxProxy gmxUtilsLogic = new GmxProxy();
bytes memory data = abi.encodeWithSelector(
GmxProxy.initialize.selector,
orderHandler,
liquidationHandler,
adlHandler,
gExchangeRouter,
gmxRouter,
dataStore,
orderVault,
gmxReader,
referralStorage
);
address gmxProxy = address(
new TransparentUpgradeableProxy(
address(gmxUtilsLogic),
address(proxyAdmin),
data
)
);
payable(gmxProxy).transfer(1 ether);
​
address keeper = makeAddr("keeper");
​
reader = new VaultReader(
orderHandler,
dataStore,
orderVault,
gmxReader,
referralStorage
);
PerpetualVault perpetualVault = new PerpetualVault();
data = abi.encodeWithSelector(
PerpetualVault.initialize.selector,
address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336), // ethUsdcMarket,
keeper,
makeAddr("treasury"),
gmxProxy,
reader,
1e8,
1e28,
10_000
);
vm.prank(address(this), address(this));
vault = payable(
new TransparentUpgradeableProxy(
address(perpetualVault),
address(proxyAdmin),
data
)
);
​
data = abi.encodeWithSelector(
GmxProxy.initialize.selector,
orderHandler,
liquidationHandler,
adlHandler,
gExchangeRouter,
gmxRouter,
dataStore,
orderVault,
gmxReader,
referralStorage
);
address gmxProxy2x = address(
new TransparentUpgradeableProxy(
address(gmxUtilsLogic),
address(proxyAdmin),
data
)
);
payable(gmxProxy2x).transfer(1 ether);
​
data = abi.encodeWithSelector(
PerpetualVault.initialize.selector,
address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336), // ethUsdcMarket,
keeper,
makeAddr("treasury"),
gmxProxy2x,
reader,
1e8,
1e28,
20_000
);
vm.prank(address(this), address(this));
vault2x = payable(
new TransparentUpgradeableProxy(
address(perpetualVault),
address(proxyAdmin),
data
)
);
​
mockData = new MockData();
}
​
function test_Deposits() external {
address user = makeAddr("user");
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
​
console.log(
"minDepositAmount is ",
PerpetualVault(vault).minDepositAmount(),
" and maxDepositAmount is ",
PerpetualVault(vault).maxDepositAmount()
);
​
PerpetualVault(vault).setMinMaxDepositAmount(
PerpetualVault(vault).maxDepositAmount(),
PerpetualVault(vault).minDepositAmount()
);
console.log(
"minDepositAmount is ",
PerpetualVault(vault).minDepositAmount(),
" and maxDepositAmount is ",
PerpetualVault(vault).maxDepositAmount()
);
​
console.log("Trying to deposit with => amount = minDepositAmount + 1 ");
uint256 amount = PerpetualVault(vault).minDepositAmount() + 1;
​
vm.startPrank(user);
deal(address(collateralToken), user, amount);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, amount);
//@audit - This will revert eventhough we gave 1 wei more than minimum deposit amount requested
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(
amount
);
vm.stopPrank();
}
}
​

Impact

Unexpected behaviour in the system. Calls get reverted. However this can be updated later by admin hence marking it as low

Deposits will fail always because they will satisfy these conditions

function deposit(uint256 amount) external nonReentrant payable {
...
if (amount < minDepositAmount) {
revert Error.InsufficientAmount();
}
if (totalDepositAmount + amount > maxDepositAmount) {
revert Error.ExceedMaxDepositCap();
}
...
}

POC results

Tools Used

Manual review

Recommendations

Below check to be added in 2 places - initialize() function and setMinMaxDepositAmount() function in PerpetualVault.sol

require(minDepositAmount < maxDepositAmount,"incorrect min and max deposit amounts" );

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Support

FAQs

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