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

6 stars 4 forks source link

0x996 - If the withdrawal account is not the caller itself, the `ChilizWrappedERC20::withdrawTo` function will encounter issues with incorrect token amount transfer. #221

Closed sherlock-admin2 closed 6 months ago

sherlock-admin2 commented 6 months ago

0x996

medium

If the withdrawal account is not the caller itself, the ChilizWrappedERC20::withdrawTo function will encounter issues with incorrect token amount transfer.

Summary:

If the withdrawal account is not the caller itself, the ChilizWrappedERC20::withdrawTo function will transfer the token amount by subtracting the burnt amount from the withdrawal amount from the caller's account to the withdrawal account. However, this is problematic because only the actual unwrapped amount unwrapAmount needs to be transferred directly, without subtracting the burnt amount burntAmount of tokens. Otherwise, incorrect token transfer issues may occur.

Vulnerability Detail

  1. If the withdrawal account is not the caller itself, asset transfer operation is executed, transferring the token amount of the withdrawal minus the burnt amount from the caller's account to the withdrawal account. _transfer(msgSender, account, amount - burntAmount);
  2. uint256 unwrapAmount = amount / decimalsOffset; and uint256 burntAmount = unwrapAmount * decimalsOffset;.
  3. When we substitute numerical values for calculation testing, we find that the final result is: amount = burntAmount. This will lead to incorrect token transfer issues in _transfer(msgSender, account, amount - burntAmount);.

Impact

If the withdrawal account is not the caller itself, it will lead to incorrect token transfer issues.

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:

Suggest changing amount - burntAmount to unwrapAmount.

if (msgSender != account) {
-    _transfer(msgSender, account, amount - burntAmount); // @audit Transfer of Token amount error.
+    _transfer(msgSender, account, unwrapAmount); 
}
nevillehuang commented 6 months ago

Invalid, there is no issue presented here. If the account has insufficient balance to transfer to approved caller withdrawing, it should revert accordingly