code-423n4 / 2022-11-size-findings

1 stars 0 forks source link

`SizeSealed.withdraw` will revert on `mulDivDown` if `a.data.lowest` is the value zero, which can prevent withdraws for an auction #315

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L377 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L381 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L384

Vulnerability details

Proof of Concept

The call to mulDivDown on L377 will revert if the third argument (denominador) is the value zero. See Solmate implementation.

It's possible for a.data.lowestBase to receive the value zero, which will prevent the transfers from L381 and L384.

Impact

If the transfers from L381 and L384 cannot the executed, for an auction with a.data.lowestBase set to zero, calling withdraw() will revert, preventing the withdrawal.

Recommended Mitigation Steps

Only call mulDivDown if a.data.lowerBase is not zero. E.g.

diff --git a/src/SizeSealed.sol b/src/SizeSealed.sol
--- a/src/SizeSealed.sol
+++ b/src/SizeSealed.sol
         // Refund unfilled quoteAmount on first withdraw
-        if (b.quoteAmount != 0) {
+        if (b.quoteAmount != 0 && a.data.lowestBase != 0) {
             uint256 quoteBought = FixedPointMathLib.mulDivDown(baseAmount, a.data.lowestQuote, a.data.lowestBase);

Alternatively, if a auction shoudn't be finalized with a.data.lowestBase set to zero, consider adding a validation on SizeSealed.finalize(). E.g.

diff --git a/src/SizeSealed.sol b/src/SizeSealed.sol
--- a/src/SizeSealed.sol
+++ b/src/SizeSealed.sol
         a.data.lowestBase = clearingBase;
+        if (clearingQuote == 0) {
+            revert InvalidClearingQuote();
+        }
         a.data.lowestQuote = clearingQuote;
trust1995 commented 1 year ago

lowestBase can never be 0 because it is set to clearingBase which is the denominator in finalize() calculation, making it revert.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid