Deecnt contracts are using same WETH interface across different chains which can disable some critical features

[c4-bot-10]

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L60
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L178
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L275

Vulnerability details

Vulnerability Details

WETH implementation is different in some chains but DecentEthRouter and DecentBridgeExecutor and several other contracts are using same interface for all different chains

Proof of Concept

WETH contract is implemented differently in some chains.

polygon mainnet:
https://polygonscan.com/address/0x7ceB23fD6bC0adD59E62ac25578270cFf1b9f619#code
deposit function of WETH has 2 parameters:

function deposit(address user, bytes calldata depositData)
    external
    override
    only(DEPOSITOR_ROLE)
{
    uint256 amount = abi.decode(depositData, (uint256));
    _mint(user, amount);
}

and all calls to this function are restricted to DEPOSITOR_ROLE, meaning that DecentEthRouter or DecentBridgeExecutor or other contracts wont be able to call this function.

Avalanche C-Chain:
https://avascan.info/blockchain/c/address/0x49D5c2BdFfac6CE2BFdB6640F4F80f226bc10bAB/contract
WETH implementation in Avalanche does not have a deposit function at all, instead it has a mint function

function mint(
    address to,
    uint256 amount,
    address feeAddress,
    uint256 feeAmount,
    bytes32 originTxId
) public {
    require(bridgeRoles.has(msg.sender), "Unauthorized.");
    _mint(to, amount);
    if (feeAmount > 0) {
        _mint(feeAddress, feeAmount);
    }
    emit Mint(to, amount, feeAddress, feeAmount, originTxId);
}

All calls to this function are also limited to bridgeRoles which means that Decent system contracts wont be able to call this function
It also does not support a withdraw function, instead it has an unwrap function:

function unwrap(uint256 amount, uint256 chainId) public {
    require(tx.origin == msg.sender, "Contract calls not supported.");
    require(chainIds[chainId] == true, "Chain ID not supported.");
    _burn(msg.sender, amount);
    emit Unwrap(amount, chainId);
}

Impact

WETH.deposit or WETH.withdraw are used in different parts of the codebase, any action that involves making a call to WETH using an incorrect interface is disabled.
Example (not limited to):
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L60
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L178
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L275

Tools Used

Manual review

Recommended Mitigation Steps

use a compatible interface for interacting with WETH in different chain.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Function call disruptions due to varied WETH interfaces cross-chain.

[c4-sponsor]

wkantaros marked the issue as disagree with severity

[c4-sponsor]

wkantaros (sponsor) confirmed

[alex-ppg]

Related to #505, an interesting problem here is that the withdraw signature does not exist on all chains but balanceOf does. As such, some permanent DoS attack could manifest although none of the submissions describe it. I will re-evaluate once I acquire Sponsor feedback on the aforementioned submission.

As-is, I definitely consider it a valid QA issue given that it is a real inconsistency that should be avoided in more explicit ways.

[alex-ppg]

Submission #429 details the discrepancies between WETH implementations greatly and I thank the Warden for their effort in detailing them.

Inspecting #505 will highlight that the WETH asset will not really be transferred in an L2 solution and can be "maliciously" forced to be transferred but would yield no exploitation vector.

This and its duplicate submissions differ in that they detail how a transaction may fail by assuming the WETH token is conformant with its Ethereum main-net interface which is incorrect.

Inspecting the codebase, we have the following invocations of non-EIP-20 functions that are "safe":

• DecentEthRouter::_bridgeWithPayload: The WETH::deposit function call is protected by the gasCurrencyIsEth flag, meaning it won't be executed in non-Ethereum chains.
• DecentEthRouter::onOFTReceived: The WETH::withdraw function call will only occur if !gasCurrencyIsEth, meaning that it won't be executed in non-Ethereum chains.
• DecentEthRouter::redeemEth: Failure of this function is desirable on non-Ethereum chains and it is not meant to be invoked normally. See Loss of funds when redeeming Decent tokens on chains that dont support ETH as gas token #505.
• DecentEthRouter::addLiquidityEth: This function is protected by the DecentEthRouter::onlyEthChain modifier and thus will not be executable in non-Ethereum chains.
• DecentEthRouter::removeLiquidityEth: This function is protected by the DecentEthRouter::onlyEthChain modifier and thus will not be executable in non-Ethereum chains.
• DecentBridgeExecutor::_executeEth: This function is invoked by DecentBridgeExecutor::execute solely when !gasCurrencyIsEth, meaning that it won't be executed in non-Ethereum chains.

Based on the above, the DecentEthRouter and DecentBridgeExecutor are "safe" and will never fail due to the non-conformity of the interfaces. I fully agree with the Wardens that this is a discrepancy that should be rectified in different, more explicit ways (i.e. different router for L2 and different for main-net) that will also save gas, however, it does not presently constitute a high or medium risk vulnerability.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid