Unable to withdraw tokens from lockbox because the burn function will revert

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/xERC20/contracts/XERC20.sol#L108-L109

Vulnerability details

mint() & burn() functions are used inside the XERC20 contract for bridges to mint & burn tokens for a user respectively.

The issue arises while burning the tokens as they will always revert when msg.sender != _user due to insufficient allowance because the tokens were not approved while minting.

Proof of Concept

The _withdraw function inside XERC20LockBox contract is used to withdraw tokens from the lockbox. It uses the burn functionality of the XERC20 contract.

File: XERC20LockBox.sol

    function _withdraw(address _to, uint256 _amount) internal {
        emit Withdraw(_to, _amount);

        XERC20.burn(msg.sender, _amount);
     ...

The burn function is used for burning the tokens of a user. According to the check, when the caller is not the user, allowance is decreased by the given amount by calling the _spendAllowance().

File: XERC20.sol

    function burn(address _user, uint256 _amount) public virtual {
        if (msg.sender != _user) {
            _spendAllowance(_user, msg.sender, _amount);       /// note - will revert here
        }

        _burnWithCaller(msg.sender, _user, _amount);
    }

Here we can see the currentAllowance would be 0 because it was not previously approved. Thus in the second if statement currentAllowance < value will always be true & ultimately revert.

File: ERC20Upgradeable.sol

   function _spendAllowance(address owner, address spender, uint256 value) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);     /// note - will return 0
        if (currentAllowance != type(uint256).max) {
            if (currentAllowance < value) {                       /// @note - will always be true & revert 
                revert ERC20InsufficientAllowance(spender, currentAllowance, value);
            }
            unchecked {
                _approve(owner, spender, currentAllowance - value, false);
            }
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Maybe set an allowance while minting

File: XERC20.sol

    function mint(address _user, uint256 _amount) public virtual {
+      if (msg.sender != _user) {
+           _approve(_user, msg.sender, _amount, true);
+       }
        _mintWithCaller(msg.sender, _user, _amount);
    }

Assessed type

Error

[DadeKuma]

It won't revert, because allowance is not checked:

This is the call:

XERC20.burn(msg.sender, _amount);

So

File: XERC20.sol

    function burn(address _user, uint256 _amount) public virtual {
        if (msg.sender != _user) {
            _spendAllowance(_user, msg.sender, _amount);
        }

        _burnWithCaller(msg.sender, _user, _amount);
    }

_user will be equal to msg.sender, and the allowance logic will not be triggered.

[DadeKuma]

@howlbot reject

[Shubh0412]

The reasoning given by the validator is incorrect.

_user will be equal to msg.sender, and the allowance logic will not be triggered.

When the call is made from the XERC20LockBox contract, the msg.sender now becomes the address of the XERC20LockBox contract itself & not the original caller of the burn function.

Say, Alice wants to withdraw some of her tokens. Right now Alice is the msg.sender.

File: XERC20LockBox.sol

        function _withdraw(address _to, uint256 _amount) internal {
            emit Withdraw(_to, _amount);

128:        XERC20.burn(msg.sender, _amount);
     ...

When the call is made from the XERC20LockBox contract to the XERC20 contract on line 128, the msg.sender is changed from Alice to the Lockbox's contract address inside the XERC20 contract.

File: XERC20.sol

    function burn(address _user, uint256 _amount) public virtual {
        if (msg.sender != _user) {            ///note - XERC20LockBox contract is the msg.sender now. Not Alice
            _spendAllowance(_user, msg.sender, _amount);       /// note - will revert here
        }

        _burnWithCaller(msg.sender, _user, _amount);
    }

& because there has been no approval given by Alice to set an allowance to anyone, the function always calls _spendAllowance() to decrease the allowance & reverts.

Also the same situation described above would take would happen in the OptimismMintableXERC20 contract.

File: OptimismMintableXERC20.sol

68:        function burn(address _from, uint256 _amount) public override(XERC20, IOptimismMintableERC20) {
69:           XERC20.burn(_from, _amount);
70:        }

[DadeKuma]

I agree that my comment could be misunderstood, so I will offer further explanation;

It won't revert, because allowance is not checked:

I was referring to this:

• @dev Can only be called by a bridge

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/xERC20/contracts/XERC20.sol#L102

If you check the bridge calls, you will see that the _user is always the msg.sender (because it's using address(this)):

https://github.com/search?q=repo%3Acode-423n4%2F2024-04-renzo+.burn%28&type=code

In that case, the _user will be equal to msg.sender, and the allowance logic will not be triggered.

The other call (from XERC20LockBox) checks the allowance, but the user would simply approve it in a separate tx? I don't see why the contract should give allowance to itself when minting, the user can do it and then call burn.

[Shubh0412]

@DadeKuma
I totally agree about the call made from the xRenzoBridge contract where the allowance check is skipped while burning from the XERC20 contract.

Just like the sponsor said in the other reported issue,

IMO can still work with the simple flow where user can 1. approve xERC20 token to lockbox. 2. call withdraw.

This would work fine when the user is an EOA.

But if the user is a smart contract who has already minted the tokens by depositing in the Lockbox, the user would not be able to burn the tokens because of missing approval & it would revert like explained above.

Now if the is able user transfers those tokens to a different address & tries to burn with appropriate approvals, then it would still revert in the _update() of ERC20Upgradeable contract because of insufficient balance of the user.

[DadeKuma]

But if the user is a smart contract who has already minted the tokens by depositing in the Lockbox, the user would not be able to burn the tokens because of missing approval & it would revert like explained above.

Yes, but that is the same impact as using mint instead of safeMint, which is usually judged as QA nowadays. Moreover, code-423n4/2024-04-renzo-findings#3 was also marked as QA.

[alcueca]

But if the user is a smart contract who has already minted the tokens by depositing in the Lockbox, the user would not be able to burn the tokens because of missing approval & it would revert like explained above.

If the user is a smart contract that can call withdraw, it will have no trouble calling approvejust before.