Limit orders can be erroneously closed
Description
One option when opening a new position is to make it a limit order, causing it to be closed as soon as one of the reserves goes to zero:
fn handle_limit_order(
&self,
prev_position: &Option<position::Position>,
position: Position,
) -> Position {
let id = position.id();
match prev_position {
Some(_) if position.close_on_fill => {
// It's technically possible for a limit order to be partially filled,
// and unfilled on the other side. In this case, we would close it prematurely.
// However, because of the arbitrage dynamics we expect that in practice an order
// gets completely filled or not at all.
if position.reserves.r1 == Amount::zero() || position.reserves.r2 == Amount::zero()
{
tracing::debug!(?id, "limit order filled, setting state to closed");
Position {
state: position::State::Closed,
..position
}
} else {
tracing::debug!(?id, "limit order partially filled, keeping open");
position
}
}
None if position.close_on_fill => {
tracing::debug!(?id, "detected a newly opened limit order");
position
}
_ => position,
}
}The issue is that this happens every time put_position is called, which also happens when trying to withdraw a position:
async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
// ... snip ...
metadata.state = position::State::Withdrawn;
state.put_position(metadata).await?;
Ok(())
}This means that if one tries to withdraw a limit order, the state will be set to Closed instead of Withdrawn.
Impact
Even though the state of the position is incorrect, withdrawing a position still requires a closed NFT to be spent. Normally this would not be possible, but due to another bug, it was possible to end up with multiple NFTs and withdraw multiple times (see Finding ref↗).
Recommendations
The handle_limit_order function should only be called when a position is in the Opened state.
Remediation
This issue has been acknowledged by Penumbra Labs, and a fix was implemented in commit 1c9452d2↗.