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

1 stars 0 forks source link

VARIABLE BALANCE TOKEN ASSOCIATED WITH LOSS AND LOCKING OF FUNDS #301

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#L163 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L351 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#L439

Vulnerability details

Impact

ERC20 tokens that are either deflationary or re-basing down could have their respective balance change. The balance could become insufficient at the time of withdraw(), refund() or cancel() to the bidders whose funds will be locked due to DOS. The way to take the fund out is to send more quoteToken into the contract which is impractical, causing fund loss to the protocol. And there is no guarantee that until the end time the balance would stay above the needed amount, hence, the lock and loss issues persist.

Proof of Concept

The problem originates here where the contract receives lower than expected amount of tokens from the bidders:

Line 163

        SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

Transferring quoteToken to the bidders in the beginning is going to go through, but as time goes on, the contract balance gets depleted and starts to fail all subsequent transfers. Consequently, withdraw(), refund() or cancel() will revert, locking bidders' fund due to DOS. The situation will transpire whether or not the seller chooses to reveal the private key.

Line 351

        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

Line 381

            SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, refundedQuote);

Line 439

        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

Recommended Mitigation Steps

Disallow variable balance tokens of this nature by implementing the same baseToken sanity check on the quoteToken.

Line 163 could be refactored to:

        uint256 balanceBeforeTransfer = ERC20(a.params.quoteToken).balanceOf(address(this));

        SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

        uint256 balanceAfterTransfer = ERC20(a.params.quoteToken).balanceOf(address(this));

        if (balanceAfterTransfer - balanceBeforeTransfer != quoteAmount) { 
            revert UnexpectedBalanceChange(); 
        }
trust1995 commented 1 year ago

Valid, dup of #255

c4-judge commented 1 year ago

0xean marked the issue as duplicate

c4-judge commented 1 year ago

0xean marked the issue as satisfactory