Loss of funds when redeeming Decent tokens on chains that dont support ETH as gas token

[c4-bot-5]

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L285-L291
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L275-L276
https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L313-L319

Vulnerability details

Impact

High - imho the impact of this issue is should be classified as high due to its substantial likelihood, simplicity in execution (requiring only a single function call), and the potential for substantial financial losses. Users face a considerable risk, as demonstrated by scenarios such as swapping 1 Ethereum for 1 Matic, resulting in significant loss of funds.

Proof of Concept

In some chains, such as Polygon/AVAX, Ethereum is not recognized as their gas token. This distinction is addressed in the DecentEthRouter by configuring the gasCurrencyIsEth variable as either true or false. However, some sections of both DecentEthRouter and DecentBridgeExecutor (as mentioned below) are mistaking Ethereum as the native chain token without checking gasCurrencyIsEth.

DecentEthRouter::redeemEth:
When a user wants to redeem their Decent tokens (on Polygon/Avax) using this function, the user's Decent tokens are transferred to the contract, WETH is withdrawn (burned), and native chain tokens (e.g., Matic in the Polygon example) are sent to the user. This implies that if, for example, Alice decides to redeem her 10 dcntEth tokens, she will receive 10 Matic tokens, which are worth only 7 USD, resulting in the loss of all her 10 WETH (worth 22,000 usd)

DecentBridgeExecutor::_executeETH:
Also in this function, WETH is withdrawn and Matic/Avax are received which might be useless when calling the target.
this function results in loss of funds for user (from) and stuck funds in DecentBridgeExecutor which can be withdrawn by a malicious actor.

NOTE: there might not be enough MATIC/AVAX in router in order to be sent to user, however, a malicious actor is able to fund contract with enough MATIC/AVAX.

Code PoC:
while the issue is clear, i have decided to put a code PoC (Copy from next line to before Tools Used and paste in a new file, no additional settings needed):

//SPDX-License-Identifier: MIT

pragma solidity ^0.8.13;

import {Test, console, console2} from "forge-std/Test.sol";
import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";

interface IWETH {
function withdraw(uint amt) external payable;

function deposit() external payable;

function balanceOf(address user) external view returns (uint);

}

contract MockRouter is Test {
IWETH polygon_weth = IWETH(0x7ceB23fD6bC0adD59E62ac25578270cFf1b9f619);
IWETH wrappedMatic = IWETH(0x0d500B1d8E8eF31E21C99d1Db9A6444d3ADf1270);

ERC20Mock dcntToken;
address alice;

modifier onlyIfWeHaveEnoughReserves(uint256 amount) {
    require(
        polygon_weth.balanceOf(address(this)) > amount,
        "not enough reserves"
    );
    _;
}

function setUp() public {
    vm.createSelectFork("https://polygon.meowrpc.com");
    alice = makeAddr("Alice");
    dcntToken = new ERC20Mock();
    dcntToken.mint(alice, 10 ether);
    //transfer WETH to router
    vm.startPrank(0x62ac55b745F9B08F1a81DCbbE630277095Cf4Be1);
    IERC20(address(polygon_weth)).transfer(address(this), 11 ether);
    vm.stopPrank();
}

function redeemEth(
    uint256 amount
) public onlyIfWeHaveEnoughReserves(amount) {
    dcntToken.transferFrom(msg.sender, address(this), amount);
    polygon_weth.withdraw(amount);
    payable(msg.sender).transfer(amount);
}

function testWithdrawWETH() public {
    //alice deciding to withdraw his 10 Dcnt tokens for ether
    assertEq(dcntToken.balanceOf(alice), 10 ether);
    assertEq(alice.balance, 0); //no matics
    assertEq(polygon_weth.balanceOf(alice), 0); //no WETH
    assertEq(wrappedMatic.balanceOf(alice), 0); //alice doesn't have any MATIC

    vm.startPrank(alice);
    dcntToken.approve(address(this), 10 ether);
    this.redeemEth(10 ether);
    wrappedMatic.deposit{value: alice.balance}();

    assertEq(dcntToken.balanceOf(alice), 0);//tokens sent to the contract
    assertEq(alice.balance, 0); //no matics
    assertEq(polygon_weth.balanceOf(alice), 0); //no WETH
    assertEq(wrappedMatic.balanceOf(alice), 10 ether); //alice has 10 matics instead of WETH 
    vm.stopPrank();
}

receive() external payable {}

}

Tools Used

Manual review
Forge testing

Mitigation steps

While I don't have a clear solution in mind yet, I would suggest first checking gasCurrencyIsEth to determine whether the chain supports ETH as the gas token or not. If it does not (false), then proceed to swap WETH for the native chain token (e.g., WETH => MATIC) and send MATIC tokens to the user. Alternatively, consider not allowing certain functions to be executed if the chain does not support ETH as the native gas token.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[raymondfam]

Would indeed be a serious issue if the vulnerability is valid.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

wkantaros (sponsor) confirmed

[alex-ppg]

The Warden demonstrates a discrepancy in the system concerning the weth token and how it is treated in the overall system.

I investigated the overall deployment system of the Sponsor and indeed validated that the DecentEthRouter is deployed with the WETH address rather than a wrapped native asset variant (i.e. wMATIC for Polygon).

The problem lies in the actual exploitability of the function paths defined. The entire system would not really behave as the Warden details.

Specifically, DecentEthRouter::redeemEth mandates that the contract has a sufficient WETH balance. Router deployments that are made outside the Ethereum network will have a 0 balance and thus the user will not be able to redeem their Decent ETH (unless someone sent WETH to the contract). This idea is flawed, as its pre-conditions are:

• Incorrect invocation of DecentEthRouter::redeemEth on a non-Ethereum network by the victim
• Malicious user sacrificing the full amount they intend to grief the victim of
• Net negative for victim and attacker of substantial amount with no apparent result that the attacker benefits from

In relation to the DecentBridgeExecutor, the code will "never" execute in non-Ethereum chains. This function is invoked solely by DecentEthRouter::onOFTReceived which would evaluate its own weth balance, detect that it does not have a sufficient one, and simply transfer the credited DcntEth to the intended _to recipient.

While overall flawed, the described execution paths behave as expected and cannot be exploited to force fund loss for the system maliciously. Cross-chain transfers of the DcntEth token will execute "correctly" as the DecentEthRouter::onOFTReceived function will end early and simply transfer the DcntEth to their intended recipient.

As a result of all this, I consider the submission invalid but the Sponsor should definitely re-evaluate how they have approached multi-chain deployments of their system as they are presently illegible and permit reverting functionality that should be inaccessible due to a more descriptive error message. I also invite the Sponsor to re-evaluate and whether they have detected a valid exploitation path based on the data supplied by the Warden.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[alex-ppg]

After discussions with the Sponsor, I stand by my original invalidation judgment due to the impossibility of exploiting the vulnerability. To note, for a user to be attacked the attacker would have to sacrifice an equal value of the funds they wish to damage the user of. This is not acceptable as a vulnerability of HM risk but rather a QA (L) issue of code misbehavior that should be prevented. Based on this, I will proceed with downgrading the submission to QA (L) as it details a valid problem in the code concerning DecentEthRouter::onOFTReceived.

[c4-judge]

alex-ppg removed the grade

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[alex-ppg]

I will award this submission a B grade as it goes into great detail about a potentially valid issue in the code. The reason this does not constitute a medium risk vulnerability is out of "luck" due to how the system is laid out, and the submission goes into great detail about all functions affected as well as their potential impact.

[c4-judge]

alex-ppg marked the issue as grade-b