DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Valid

Owner can arbitrary change the withdrawal locktime period in PerpetualVault :: setLockTime, leading to users been trapped

[M-1] Owner can arbitrary change the withdrawal locktime period in PerpetualVault :: setLockTime, leading to users been trapped

Vulnerability Details:
In Defi mistakes always happens and is neccesary that protocols do not wait for it to lead to that because the harm will already been done ,

In PerpetualVault :: setLockTime Owner is allowed to arbitrary change lockTime period, owner can mistakenly or maliciously set the locktime period to something way higher than intended

is neccesary to have max locktime since this deals with getting out funds which is beneficial to users, if this kind of mistake is done it can break users trust in the protocol,

this issue does not just affects 1 user it will affects all users which is why the impact is high

Impact:
Users wont be able to withdraw if lock time is set wrongly

proof of code

function testLockTimeArbitraryExtensionCanSeizeUsersFunds() public {
// Setup: User deposits
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
address mike = makeAddr("mike");
uint256 lockTime = 7 days;
uint256 newLockTime = 365 days;
// Set the lockTime to 7 days
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(lockTime);
// Get USDC token and setup deposit
address whale = 0x489ee077994B6658eAfA855C308275EAd8097C4A;
vm.startPrank(whale);
uint256 amount = 1e10;
collateralToken.transfer(mike, amount);
vm.stopPrank();
// mike Creating deposit
vm.startPrank(mike);
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 + 3);
// Owner extends lock to 1 year
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(newLockTime); // Set to 1 year
// User's withdrawal now fails , despite waiting for initial 7 days period vm.startPrank(mike);
PerpetualVault(vault).withdraw(mike, 1);
vm.stopPrank();
}

vulnerable code

function setLockTime(uint256 _lockTime) external onlyOwner {
lockTime = _lockTime;
}

Recommended Mitigation:
Add maxLockTime, this will make sure the lockTime stays within a threshold

function setLockTime(uint256 _lockTime) external onlyOwner {
require(_lockTime < maxLockTime, "lockTime pass Threshold");
lockTime = _lockTime;
}
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."

Appeal created

n0kto Lead Judge
4 months ago
n0kto Lead Judge 4 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.