Confusing math in function finalizeVaultEndedWithdrawals
The function finalizeVaultEndedWithdrawals finalizes the withdrawals for the vault ending. In it, there is some math:
function finalizeVaultEndedWithdrawals(uint256 side) external {
// [...]
vaultEndedFixedDepositsFunds = claimWithdrawals(msg.sender, vaultEndedWithdrawalRequestIds);
uint256 fixedETHDeposit = fixedETHDepositTokenTotalSupply;
if (fixedETHDeposit < vaultEndedFixedDepositsFunds) {
vaultEndedStakingEarnings = vaultEndedFixedDepositsFunds - fixedETHDeposit;
vaultEndedFixedDepositsFunds -= vaultEndedStakingEarnings;
}
// [...]
}This math is confusing and needlessly circuitous. In the case where fixedETHDeposit < vaultEndedFixedDepositsFunds, the second line subtracts vaultEndedStakingEarnings from vaultEndedFixedDepositsFunds, but the former quantity is the latter quantity minus fixedETHDeposit, which means it is equivalent to just setting it to fixedEthDeposit.
Moreover, reading and writing storage costs more gas than local variables. We recommend refactoring this math to an equivalent calculation that is more clear and manipulates storage less, such as this:
uint256 amountWithdrawn = claimWithdrawals(msg.sender, vaultEndedWithdrawalRequestIds);
uint256 fixedETHDeposit = fixedETHDepositTokenTotalSupply;
if (amountWithdrawn > fixedETHDeposit) {
vaultEndedStakingEarnings = amountWithdrawn - fixedETHDeposit;
vaultEndedFixedDepositsFunds = fixedETHDeposit;
} else {
vaultEndedFixedDepositsFunds = amountWithdrawn;
}This issue has been acknowledged by Saffron, and a fix was implemented in commit 3552ae7e↗.