Description

When opening a new position, the check_stateful handler ensures that there is not an existing position with the same ID:

async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
    // Validate that the position ID doesn't collide
    state.check_position_id_unused(&self.position.id()).await?;

    Ok(())
}

As all the check_stateful checks are run in parallel (see ref↗), if two positions are opened in the same transaction, then it is possible to open the same position multiple times. This allows one to receive two or more NFTs for the same position ID, even though they may have opened them with different reserves.

Impact

A malicious user could exploit this bug to withdraw more than the initial reserves through the following:

1. In the same transaction, open two identical positions but with the first having a reserve of 1upenumbra and the other 100penumbra.

2. They now have two NFTs for the position, but the reserve for the position has been set to 100penumbra.

3. Close both positions, and they will now have two NFTs for the closed position.

4. Withdraw the position, due to the bug in handle_limit_order (see ref↗); the position will be set to Closed instead of Withdrawn.

5. Withdraw the position again, and they will now have withdrawn 200penumbra.

Recommendations

When adding a new position to the state, there should be a check to ensure that the position ID has not already been used.

Remediation

This issue has been acknowledged by Penumbra Labs, and fixes were implemented in the following commits:

• 1f084185↗
• 2949f8f0↗