code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/xETH.sol#L9

Vulnerability details

Impact

anyone can burn without MINTER_ROLE

Proof of Concept

xETH we will restrict mint/burn to only those with MINTER_ROLE privileges The documentation is commented as follows:

It has Access Control, so anyone with MINTER_ROLE can mint xETH. In this specific case, MINTER_ROLE is assigned to AMO contract. For minting / burning xETH from the Curve Pool.

    function burnShares(
        uint256 amount
    ) public onlyRole(MINTER_ROLE) whenNotPaused {   //<------@audit only MINTER_ROLE
        /// @dev if the amount to be burned is 0, revert.
        if (amount == 0) revert AmountZeroProvided();
        _burn(msg.sender, amount);
    }

But currently xETH is ERC20Burnable, and there is no MINTER_ROLE restriction in ERC20Burnable.burn()/burnFrom(). Then anyone can burn, breaking this restriction rule

contract xETH is ERC20, ERC20Burnable, Pausable, AccessControl {
....

abstract contract ERC20Burnable is Context, ERC20 {
...
    function burn(uint256 amount) public virtual {
        _burn(_msgSender(), amount);
    }
    function burnFrom(address account, uint256 amount) public virtual {
        _spendAllowance(account, _msgSender(), amount);
        _burn(account, amount);
    }

Tools Used

Recommended Mitigation Steps

remove is ERC20Burnable

Assessed type

Context

romeroadrian commented 1 year ago

Mentioned as low here https://github.com/code-423n4/2023-05-xeth-findings/blob/main/data/adriro-Q.md#l-4-potential-accidental-inclusion-of-erc20burnable-in-xeth-token . Only the holder or the allowed spender can burn their tokens.

c4-sponsor commented 1 year ago

vaporkane marked the issue as disagree with severity

c4-sponsor commented 1 year ago

vaporkane marked the issue as sponsor confirmed

kirk-baird commented 1 year ago

This is a valid edge cases that users can burn their own tokens. However, there is no benefit to burn ones own tokens and just results in net gains for other users (as share price increases). As a result I consider this Low severity.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)