XERC20Lockbox#withdraw() currently does not work as expected since it reverts in the attempt to burn the requested XERC20 tokens to be withdrawn

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20Lockbox.sol#L103-L137
https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20.sol#L107-L114

Vulnerability details

Proof of Concept

Take a look at the current logic applied to withdrawals in the XERC20Lockbox https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20Lockbox.sol#L103-L137

    function withdraw(uint256 _amount) external {
        _withdraw(msg.sender, _amount);
    }

    function withdrawTo(address _to, uint256 _amount) external {
        _withdraw(_to, _amount);
    }

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

        XERC20.burn(msg.sender, _amount);

        if (IS_NATIVE) {
            (bool _success, ) = payable(_to).call{ value: _amount }("");
            if (!_success) revert IXERC20Lockbox_WithdrawFailed();
        } else {
            ERC20.safeTransfer(_to, _amount);
        }
    }

These functions are eventually called whenever there is a need to withdraw ERC20 tokens from the lockbox and from the snippet attached above this, just in it's sense is implemented like a normal withdrawal approach and should work without problems, issue here however is with the way the logic is held up on before the call to burn the requested tokens to be withdrawn via XERC20.burn(msg.sender, _amount);, if we check the implementation of XERC20.burn() at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20.sol#L107-L114

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

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

We can see that in the case where the (msg.sender != _user) the execution attempts to spend the allowance the user has given to the msg.sender, would be key to note that from our withdrawal attempt via XERC20Lockbox#withdraw() in XERC20.burn() the msg.sender() is always not going to be the user since msg.sender is now the XERC20Lockbox.sol contract. Back to the logic present in XERC20Lockbox#withdraw(), we can see that before querying XERC20.burn(), no approval is set for the lockbox contract to spend the user's amount of tokens to be withdrawn neither are the tokens transferred in and then directly burnt, which then means that all attempts to withdraw the tokens would fail, cause when the call gets to the burn execution in XERC20.sol this attempt to spend allowance would always revert.

Impact

Functionality of protocol's core logic is flawed, and it's availability is affected, considering the current code scope, all attempts to withdraw tokens from the XERC20 lockbox would encounter a revert.

Recommended Mitigation Steps

A recommended fix could be to first transfer in the XERC20 tokens from the users during the attempt of withdrawal and then directly burn these tokens from the lockbox itself, ensuring that the attempt would never revert, so consider applying these changes to https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20Lockbox.sol#L117-L137

    function _withdraw(address _to, uint256 _amount) internal {
        emit Withdraw(_to, _amount);
+ 
+       XERC20.transferFrom(msg.sender, address(this), _amount);
- 
-       XERC20.burn(msg.sender, _amount);
+ 
+       XERC20.burn(address(this), _amount);

        if (IS_NATIVE) {
            (bool _success, ) = payable(_to).call{ value: _amount }("");
            if (!_success) revert IXERC20Lockbox_WithdrawFailed();
        } else {
            ERC20.safeTransfer(_to, _amount);
        }
    }

Assessed type

Token-Transfer

[jatinj615]

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

[alcueca]

Taking into account that xERC20 uses permit, the impact seems minimal.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as grade-b