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

Unrestricted Lock Time Modifications Can Force-Lock User Deposits

codertjay

Summary

The setLockTime function allows the owner to arbitrarily modify the withdrawal locktime period without any restrictions,
potentially trapping users' funds by extending the lock duration for existing deposits.

Currently we know the owner has full permission to make any update but still that doesn't mean if the owner account get leaked users could loose thier money or when the owner feel like he can prevent users from taking out their cash.

This current modifications could allow and existing users order to have more delay.

For example taking naruto as user and madara as owner

  • naruto(user) deposit funds when locktime is 7 days

  • madara(owner) increase lock time to 2 years

  • it shouldn't affect existing deposit like the user naruto (user) because if madara(owner) increases this time then naruto cant withdraw his funds

Vulnerability Details

// In initialization
lockTime = 7 * 24 * 3600; // 1 week initially
​
// Can be changed anytime by owner
function setLockTime(uint256 _lockTime) external onlyOwner {
lockTime = _lockTime;
}
​
// Used in withdraw to check lock period
function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {
revert Error.Locked();
}
// ...
}

POC:

The test illustrates a serious vulnerability in the setLockTime function where the owner can maliciously modify the lock
duration after users have deposited funds - even if users have waited through their original lock period.

For example, when a user deposits with a 7-day lock and waits the full period, the owner can extend the lock to 730 days (2 years) just before the user attempts to withdraw, effectively trapping their funds for an additional 2 years with no recourse.

This violates users' expectations about withdrawal timeframes and could be used maliciously to prevent withdrawals indefinitely.

​
function test_LockTimeExtensionTrapsUserFunds() public {
// Setup: User deposits
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
address bob = makeAddr("bob");
​
// Set the lockTime to 7 days
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(7 days); // Set to 2 years
​
​
////////////////////////////////////////////////////////////
// Get USDC token and setup deposit
address whale = 0x489ee077994B6658eAfA855C308275EAd8097C4A;
vm.startPrank(whale);
uint256 amount = 1e10;
collateralToken.transfer(bob, amount);
vm.stopPrank();
​
// Create deposit and order
vm.startPrank(bob);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
​
//////////////////////////////////////////////////////////////////
​
​
// Fast forward past original 7 day lock
vm.warp(block.timestamp + 7 days + 1);
​
// Owner extends lock just before user tries to withdraw
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(730 days); // Set to 2 years
​
// User's withdrawal now fails despite waiting original period
vm.startPrank(bob);
PerpetualVault(vault).withdraw(bob, 1);
vm.stopPrank();
}

The foundry shell response

➜ 2025-02-gamma git:(main) ✗ forge test --mt test_LockTimeExtensionTrapsUserFunds -vvvv --via-ir --rpc-url arbitrum
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/PerpetualVault.t.sol:PerpetualVaultTest
[FAIL: Locked()] test_LockTimeExtensionTrapsUserFunds() (gas: 518740)
.........
├─ [2846] TransparentUpgradeableProxy::fallback() [staticcall]
│ ├─ [1426] PerpetualVault::owner() [delegatecall]
│ │ └─ ← [Return] PerpetualVaultTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
│ └─ ← [Return] PerpetualVaultTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
├─ [0] VM::prank(PerpetualVaultTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [6614] TransparentUpgradeableProxy::fallback(63072000 [6.307e7])
│ ├─ [5194] PerpetualVault::setLockTime(63072000 [6.307e7]) [delegatecall]
│ │ └─ ← [Return]
│ └─ ← [Return]
├─ [0] VM::startPrank(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e])
│ └─ ← [Return]
├─ [49372] TransparentUpgradeableProxy::fallback(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e], 1)
│ ├─ [47945] PerpetualVault::withdraw(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e], 1) [delegatecall]
│ │ └─ ← [Revert] Locked()
│ └─ ← [Revert] Locked()
└─ ← [Revert] Locked()
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.92ms (1.32ms CPU time)
​
Ran 1 test suite in 3.85s (9.92ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/PerpetualVault.t.sol:PerpetualVaultTest
[FAIL: Locked()] test_LockTimeExtensionTrapsUserFunds() (gas: 518740)
​
Encountered a total of 1 failing tests, 0 tests succeeded

Impact

HIGH - Because:

  1. Affects all existing deposits

  2. Can trap user funds for arbitrary periods

  3. No maximum limit on lock duration

Tools Used

  • Manual code review

  • Foundry testing framework

Recommendations

  1. Add restriction that lock changes only apply to new deposits:

mapping(uint256 => uint256) public depositLockTime;
​
function setLockTime(uint256 _lockTime) external onlyOwner {
require(_lockTime <= MAX_LOCK_TIME, "Exceeds maximum lock");
lockTime = _lockTime;
}
​
function deposit(uint256 amount) external payable {
// ... other checks ...
depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0));
depositLockTime[counter] = lockTime; // Store current lock time with deposit
}
​
function withdraw(address recipient, uint256 depositId) public {
if (depositInfo[depositId].timestamp + depositLockTime[depositId] >= block.timestamp) {
revert Error.Locked();
}
// ... rest of withdraw logic
}

  1. Or add maximum lock time limit:

uint256 public constant MAX_LOCK_TIME = 30 days;
​
function setLockTime(uint256 _lockTime) external onlyOwner {
require(_lockTime <= MAX_LOCK_TIME, "Exceeds maximum lock");
lockTime = _lockTime;
}

This ensures:

  1. Lock changes don't affect existing deposits, or

  2. Lock time cannot exceed reasonable limits

  3. Users have certainty about their withdrawal timeframes

Updates

Lead Judging Commences

n0kto Lead Judge 6 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 6 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."

Appeal created

n0kto Lead Judge
5 months ago
n0kto Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_changing_lockTime_impact_previous_depositors

Likelihood: Low, when admin changes lockTime setting. Impact: Informational/Low, it will change the lockTime for previous depositors, forcing them to wait longer than expected or allowing them to withdraw earlier. This is indeed a strange implementation and is not specified in the documentation. It deserves a low.

Support

FAQs

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