code-423n4 / 2024-06-krystal-defi-findings

0 stars 0 forks source link

The BSC chain doesn't have a WETH wrapper which would cause issues for protocol's deployment on the chain #6

Closed c4-bot-10 closed 2 months ago

c4-bot-10 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L522-L533

Vulnerability details

Proof of Concept

First take a look at this excerpt from the readMe https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/README.md#L101

| Question                                | Answer                                      |
| --------------------------------------- | ------------------------------------------- |
| Chains the protocol will be deployed on | Ethereum,Arbitrum,Base,BSC,Optimism,Polygon |

We can see that a supported chain where Protocol plans on deploying to is the Binance smart chain.

Now, in multiple instances across contracts in scope we can see that there is an attempt to withdraw directly from the WETH implementation, i.e see how tokens are being transferred, by first unwrapping and then sending the ETH https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L522-L533

    function _transferToken(IWETH9 weth, address to, IERC20 token, uint256 amount, bool unwrap) internal {
        if (address(weth) == address(token) && unwrap) {
            //@audit
            weth.withdraw(amount);
            (bool sent, ) = to.call{value: amount}("");
            if (!sent) {
                revert EtherSendFailed();
            }
        } else {
            SafeERC20.safeTransfer(token, to, amount);
        }
    }

Other instances across scope where this function gets used can be seen below

ib/v3-periphery/contracts/V3Migrator.sol:
  74              if (params.refundAsETH && params.token0 == WETH9) {
  75:                 IWETH9(WETH9).withdraw(refund0);
  76                  TransferHelper.safeTransferETH(msg.sender, refund0);

  87              if (params.refundAsETH && params.token1 == WETH9) {
  88:                 IWETH9(WETH9).withdraw(refund1);
  89                  TransferHelper.safeTransferETH(msg.sender, refund1);

lib/v3-periphery/contracts/base/PeripheryPayments.sol:
  23          if (balanceWETH9 > 0) {
  24:             IWETH9(WETH9).withdraw(balanceWETH9);
  25              TransferHelper.safeTransferETH(recipient, balanceWETH9);

lib/v3-periphery/contracts/base/PeripheryPaymentsWithFee.sol:
  25          if (balanceWETH9 > 0) {
  26:             IWETH9(WETH9).withdraw(balanceWETH9);
  27              uint256 feeAmount = (balanceWETH9 * feeBips) / 10_000;

Where as this implementation works on most of the chains where the protocol is to deploy to, the query to .withdraw() would always fail on the Binance smart chain. This is because no WETH wrapper exists on BSC, also going to this helper site to get the addresses for WETH: https://www.coingecko.com/en/coins/weth we can see the different addresses for different chains, i.e

Using the EVM contract reader tool: https://www.contractreader.io/contract, we can see that, whereas the .withdraw() function exists on the implementation on the mainnet, it doesn't exist on the BSC implementation.

Impact

Functionalities of the protocol & their availability would be impacted on the BSC chain where it's scheduled to be deployed, considering the instances where IWETH.withdraw() are currently queried in scope are from helper functions like _transferToken that get used in other core functionalities, see instances where _transferToken alone is being used in scope, with this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-06-krystal-defi%20_transferToken&type=code. Which showcases how not only swaps and their preparations via Common.sol would be bricked, but even executions in the V3Automation.sol

Recommended Mitigation Steps

Reconsider the deployment on BSC

Assessed type

Context

3docSec commented 2 months ago

Provisionally marking as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as selected for report

Haupc commented 2 months ago

BSC chain have WBNB which is warped native with full function that our contract use

3docSec commented 2 months ago

Invalid as per sponsor comment

c4-judge commented 2 months ago

3docSec marked the issue as unsatisfactory: Invalid