search

sherlock-audit / 2024-02-jala-swap-judging

6 stars 4 forks source link

0x996 - In the `ChilizWrappedERC20::withdrawTo` function, if the transfer fails, it will result in the tokens being incorrectly burnt first. #230

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

0x996

medium

In the ChilizWrappedERC20::withdrawTo function, if the transfer fails, it will result in the tokens being incorrectly burnt first.

Summary:

In the ChilizWrappedERC20::withdrawTo function, it is observed that the business logic involves burning tokens first and then transferring them. If the token transfer fails, the entire transaction is rolled back, which will result in the previous tokens being incorrectly burnt.

Vulnerability Detail

Error scenario:

  1. The user wants to call withdrawTo to withdraw a certain amount of tokens and transfer them to a specified account.
  2. However, it is required to burn a certain amount of tokens first and then transfer a certain amount of tokens to the specified account.
  3. At this point, if the token transfer fails and the transaction is rolled back, it will result in the previous tokens being incorrectly burnt.

Impact

If the token transfer fails, it will indeed result in the previous tokens being incorrectly burnt.

Code Snippet:

https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/utils/ChilizWrappedERC20.sol#L45-L55

function withdrawTo(address account, uint256 amount) public virtual returns (bool) {
    if (address(underlyingToken) == address(0)) revert NotInitialized();
    uint256 unwrapAmount = amount / decimalsOffset;
    if (unwrapAmount == 0) revert CannotWithdraw();
    address msgSender = _msgSender();
    uint256 burntAmount = unwrapAmount * decimalsOffset;
@>    _burn(msgSender, burntAmount);
@>    SafeERC20.safeTransfer(underlyingToken, account, unwrapAmount);
    if (msgSender != account) {
        _transfer(msgSender, account, amount - burntAmount);
    }

Tool used

Manual Review

Recommendation:

The correct order should indeed be to transfer the tokens first and then proceed with burning them.

function withdrawTo(address account, uint256 amount) public virtual returns (bool) {
    if (address(underlyingToken) == address(0)) revert NotInitialized();
    uint256 unwrapAmount = amount / decimalsOffset;
    if (unwrapAmount == 0) revert CannotWithdraw();
    address msgSender = _msgSender();
    uint256 burntAmount = unwrapAmount * decimalsOffset;
-   _burn(msgSender, burntAmount);
    SafeERC20.safeTransfer(underlyingToken, account, unwrapAmount);
+   _burn(msgSender, burntAmount);
    if (msgSender != account) {
        _transfer(msgSender, account, amount - burntAmount);
    }
nevillehuang commented 6 months ago

Invalid, transactions are atomic, if safeTransfer fails, shares burned will revert as seen here