Summary
The PerpetualVault contract is vulnerable to a race condition in its deposit mechanism, where multiple deposits can be processed simultaneously during the period between deposit initiation and finalization of the position increase.
Vulnerability Details
function deposit(uint256 amount) external nonReentrant payable {
_noneFlow();
if (depositPaused == true) {
revert Error.Paused();
}
if (amount < minDepositAmount) {
revert Error.InsufficientAmount();
}
if (totalDepositAmount + amount > maxDepositAmount) {
revert Error.ExceedMaxDepositCap();
}
flow = FLOW.DEPOSIT;
collateralToken.safeTransferFrom(msg.sender, address(this), amount);
counter++;
depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0));
totalDepositAmount += amount;
EnumerableSet.add(userDeposits[msg.sender], counter);
if (positionIsClosed) {
MarketPrices memory prices;
_mint(counter, amount, false, prices);
_finalize(hex'');
} else {
_payExecutionFee(counter, true);
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}
}
Specific issues:
State management
FLOW public flow;
uint256 public counter;
mapping(uint256 => DepositInfo) public depositInfo;
The flow and counter variables are global state that can be accessed by multiple transactions without any adequate locking mechanism to prevent multiple simultaneous deposits.
Flow control
function _noneFlow() internal view {
if (flow != FLOW.NONE) {
revert Error.FlowInProgress();
}
}
Even though there is a _noneFlow check, this is not enough to prevent race conditions, and the check and state changes are not atomic.
Technical causes:
Asynchronous nature
if (positionIsClosed) {
MarketPrices memory prices;
_mint(counter, amount, false, prices);
_finalize(hex'');
} else {
_payExecutionFee(counter, true);
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}
The deposit process has a different asynchronous path without any mechanism that guarantees the atomicity of the entire process.
Shared state
totalDepositAmount += amount;
EnumerableSet.add(userDeposits[msg.sender], counter);
Multiple state variables are updated in a single flow without any rollback mechanism if one of the operations fails.
Related issues:
function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;
}
The _mint function also updates multiple state variables without any protection against concurrent access.
Impact
Scenario
Transaction 1:
1. Check _noneFlow() ✓
2. flow = FLOW.DEPOSIT
3. counter++
4. Set depositInfo
5. Process deposit
Transaction 2:
1. Check _noneFlow() ✓
2. flow = FLOW.DEPOSIT
3. counter++
4. Set depositInfo
5. Process deposit
POC
Add this to PerpetualVault.t.sol and run it forge test --match-test test_RaceConditionDeposit --rpc-url arbitrum -vvvv.
function test_RaceConditionDeposit() external {
address userA = makeAddr("userA");
address userB = makeAddr("userB");
uint256 depositAmount = 1000e6;
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
deal(address(collateralToken), userA, depositAmount);
deal(address(collateralToken), userB, depositAmount);
deal(userA, 2 ether);
deal(userB, 2 ether);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true) * tx.gasprice;
vm.startPrank(userA);
collateralToken.approve(vault, depositAmount);
vm.stopPrank();
vm.startPrank(userB);
collateralToken.approve(vault, depositAmount);
PerpetualVault(vault).deposit{value: executionFee}(depositAmount);
vm.stopPrank();
vm.startPrank(userA);
PerpetualVault(vault).deposit{value: executionFee}(depositAmount);
vm.stopPrank();
uint256[] memory userADeposits = PerpetualVault(vault).getUserDeposits(userA);
uint256[] memory userBDeposits = PerpetualVault(vault).getUserDeposits(userB);
assertTrue(userBDeposits[0] < userADeposits[0], "Race condition: B's later tx got earlier ID");
(, , address ownerA, , , ) = PerpetualVault(vault).depositInfo(userADeposits[0]);
(, , address ownerB, , , ) = PerpetualVault(vault).depositInfo(userBDeposits[0]);
assertEq(ownerB, userB, "B's deposit recorded first");
assertEq(ownerA, userA, "A's deposit recorded second");
emit log_named_uint("User B actual deposit ID (should be higher)", userBDeposits[0]);
emit log_named_uint("User A actual deposit ID (should be lower)", userADeposits[0]);
assertEq(PerpetualVault(vault).totalDepositAmount(), depositAmount * 2, "Total deposit amount should be correct");
}
Result:
├─ emit Minted(depositId: 1, depositor: userB, shareAmount: 100000000000000000, depositAmount: 1000000000)
├─ emit Minted(depositId: 2, depositor: userA, shareAmount: 100000000000000000, depositAmount: 1000000000)
Reverse order of execution.
├─ [2396] TransparentUpgradeableProxy::fallback(userA) [staticcall]
│ └─ ← [Return] [2]
├─ [2396] TransparentUpgradeableProxy::fallback(userB) [staticcall]
│ └─ ← [Return] [1]
Verify deposit ID.
├─ emit log_named_uint(key: "User B actual deposit ID (should be higher)", val: 1)
├─ emit log_named_uint(key: "User A actual deposit ID (should be lower)", val: 2)
Confirmation in log.
├─ [1974] TransparentUpgradeableProxy::fallback(1) [staticcall]
│ └─ ← [Return] 1000000000, 100000000000000000, userB, 0, 1737732296, 0x000...
├─ [1974] TransparentUpgradeableProxy::fallback(2) [staticcall]
│ └─ ← [Return] 1000000000, 100000000000000000, userA, 0, 1737732296, 0x000...
Verify deposit ownership.
Logs:
User B actual deposit ID (should be higher): 1
User A actual deposit ID (should be lower): 2
Tools Used
Recommendations
Implement a two-phase deposit process with proper synchronization.
contract PerpetualVault {
mapping(address => uint256) public pendingDeposits;
uint256 public constant DEPOSIT_LOCK_PERIOD = 1;
function initiateDeposit() external payable {
require(pendingDeposits[msg.sender] == 0, "Pending deposit exists");
pendingDeposits[msg.sender] = block.timestamp;
}
function finalizeDeposit(uint256 amount) external nonReentrant {
require(pendingDeposits[msg.sender] + DEPOSIT_LOCK_PERIOD <= block.timestamp,
"Deposit still locked");
require(pendingDeposits[msg.sender] != 0, "No pending deposit");
delete pendingDeposits[msg.sender];
require(flow == FLOW.NONE, "Flow in progress");
}
}
