Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anyone can update the address of the Router in the DcntEth contract to any address they would like to set. #721

Open
c4-bot-8 opened this issue Jan 23, 2024 · 6 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working H-01 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DcntEth.sol#L20-L22

Vulnerability details

Impact

By allowing anybody to set the address of the Router contract to any address they want to set it allows malicious users to get access to the mint and burn functions of the DcntEth contract.

Proof of Concept

The DcntEth::setRouter() function has not an access control to restrict who can call this function. This allows anybody to set the address of the router contract to any address they'd like to set it.

DcntEth.sol

//@audit-issue => No access control to restrict who can set the address of the router contract
function setRouter(address _router) public {
    router = _router;
}

The functions DcntEth::mint() function & DcntEth::burn() function can be called only by the router contract.

DcntEth.sol

    //@audit-info => Only the router can call the mint()
    function mint(address _to, uint256 _amount) public onlyRouter {
        _mint(_to, _amount);
    }

    //@audit-info => Only the router can call the burn()
    function burn(address _from, uint256 _amount) public onlyRouter {
        _burn(_from, _amount);
    }

A malicious user can set the address of the router contract to an account of their own and:

  1. Gain access to mint unlimited amounts of DcntEth token, which could be used to disrupt the crosschain accounting mechanism, or to steal the deposited weth in the DecentEthRouter contract.
  2. Burn all the DcntEth tokens that were issued to the DecentEthRouter contract when liquidity providers deposited their WETH or ETH into it.
  3. Cause a DoS to the add and remove liquidity functions of the DecentEthRouter contract. All of these functions end up calling the DcntEth::mint() function or the DcntEth::burn() function, if the router address is set to be different than the address of the DecentEthRouter, all the calls made from the DecentEthRouter to the DcnEth contract will revert.

DecentEthRouter.sol

    /// @inheritdoc IDecentEthRouter
    function addLiquidityEth()
        public
        payable
        onlyEthChain
        userDepositing(msg.value)
    {
        weth.deposit{value: msg.value}();
        
        //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.mint(address(this), msg.value);
    }

    /// @inheritdoc IDecentEthRouter
    function removeLiquidityEth(
        uint256 amount
    ) public onlyEthChain userIsWithdrawing(amount) {

      //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.burn(address(this), amount);
        weth.withdraw(amount);
        payable(msg.sender).transfer(amount);
    }

    /// @inheritdoc IDecentEthRouter
    function addLiquidityWeth(
        uint256 amount
    ) public payable userDepositing(amount) {
        weth.transferFrom(msg.sender, address(this), amount);

        //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.mint(address(this), amount);
    }

    /// @inheritdoc IDecentEthRouter
    function removeLiquidityWeth(
        uint256 amount
    ) public userIsWithdrawing(amount) {

      //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.burn(address(this), amount);
        weth.transfer(msg.sender, amount);
    }

Tools Used

Manual Audit

Recommended Mitigation Steps

Make sure to add an Acess Control mechanism to limit who can set the address of the Router in the DcnEth contract.

Assessed type

Access Control

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 23, 2024
c4-bot-8 added a commit that referenced this issue Jan 23, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 25, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #14

@c4-judge
Copy link

c4-judge commented Feb 3, 2024

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 3, 2024
@alex-ppg
Copy link

alex-ppg commented Feb 3, 2024

This and all relevant submissions correctly specify that the lack of access control in the DcntEth::setRouter function can be exploited maliciously and effectively compromise the entire TVL of the Decent ETH token.

A high-risk severity is appropriate, and this submission was selected as the best due to detailing all possible impacts:

  • Arbitrary mints of the token to withdraw funds provided as liquidity to UTB
  • Arbitrary burns to sabotage liquidity pools and other escrow-based contracts
  • Sabotage of liquidity provision function invocations

@c4-judge
Copy link

c4-judge commented Feb 3, 2024

alex-ppg marked the issue as selected for report

@c4-judge c4-judge reopened this Feb 3, 2024
@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Feb 3, 2024
@C4-Staff C4-Staff added the H-01 label Feb 9, 2024
@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 13, 2024
@c4-sponsor
Copy link

wkantaros (sponsor) confirmed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working H-01 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants