Description

If a vault has many depositors, when a vault's time period has ended and then its withdrawal is finalized, it is likely that multiple depositors want to withdraw. To do this, the first such withdrawer must call finalizeVaultEndedWithdrawals:

function finalizeVaultEndedWithdrawals(uint256 side) external {
  require(side == FIXED || side == VARIABLE, "IS");
  require(vaultEndedWithdrawalRequestIds.length != 0 && !vaultEndedWithdrawalsFinalized, "WNR");

  vaultEndedWithdrawalsFinalized = true;

  // [...] [actually finalize the withdrawal]

  return vaultEndedWithdraw(side);
}

And then after the first withdrawer, the rest of them must call withdraw, which directly calls vaultEndedWithdraw.

However, if the withdrawers do not coordinate off chain, then multiple withdrawers could issue a transaction for finalizeVaultEndedWithdrawals thinking that they are the first one in the next block to request withdrawal. In this case, all of the calls will revert except the first one.

Impact

The design of finalizeVaultEndedWithdrawals and withdraw causes a coordination problem when many depositors are anticipating withdrawing after the finalizing of a vault that has ended that may waste gas and time from withdrawers.

Recommendations

Instead of reverting if the vault-end withdrawal has already been finalized, call vaultEndedWithdraw so that the user's withdrawal still goes through.

Alternatively, instead of having this be a separate function that needs to be specifically called on the first withdrawal, integrate this logic with the vaultEndedWithdraw flow such that users need to only call withdraw and the first such transaction that ends up in the block sorts out the withdrawal.

Or, if the suggestion in Discussion ref↗ is implemented, then this would not be an issue since the ending of a vault would not require an asynchronous use of the withdrawal queue.

Remediation

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

• 1e5ae48b↗
• 13b8a030↗