Setting _proxyAdmin as smart contract will leads to issue in xerc20 cross-chain flow.

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/Connext/integration/LockboxAdapterBlast.sol#L69
https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/xERC20/contracts/XERC20Factory.sol#L141

Vulnerability details

Impact

Setting _proxyAdmin as smart contract will leads to issue in xerc20 cross-chain flow.

Proof of Concept

When deploying deployXERC20, we are calling

 /**
     * @notice Deploys an XERC20 contract using CREATE3
     * @dev _limits and _minters must be the same length
     * @param _name The name of the token
     * @param _symbol The symbol of the token
     * @param _minterLimits The array of limits that you are adding (optional, can be an empty array)
     * @param _burnerLimits The array of limits that you are adding (optional, can be an empty array)
     * @param _bridges The array of bridges that you are adding (optional, can be an empty array)
     * @param _proxyAdmin The address of the proxy admin - will have permission to upgrade the lockbox (should be a dedicated account or contract to manage upgrades)
     * @return _xerc20 The address of the xerc20
     */

    function deployXERC20(
        string memory _name,
        string memory _symbol,
        uint256[] memory _minterLimits,
        uint256[] memory _burnerLimits,
        address[] memory _bridges,
        address _proxyAdmin
    ) external returns (address _xerc20) {
        _xerc20 = _deployXERC20(
            _name,
            _symbol,
            _minterLimits,
            _burnerLimits,
            _bridges,
            _proxyAdmin
        );

        emit XERC20Deployed(_xerc20);
    }

note,

_proxyAdmin The address of the proxy admin - will have permission to upgrade the lockbox (should be a dedicated account or contract to manage upgrades)

and this is how the xerc20 token is deployed

        bytes memory _creation = type(TransparentUpgradeableProxy).creationCode;

        // Constructor in Proxy takes (logic, admin, data)
        bytes memory _bytecode = abi.encodePacked(
            _creation,
            abi.encode(xerc20Implementation, _proxyAdmin, initializeBytecode)
        );

        _xerc20 = CREATE3.deploy(_salt, _bytecode, 0);

        EnumerableSetUpgradeable.add(_xerc20RegistryArray, _xerc20);

create3 is meant to create deterministic adddress deployment,

for example, the blast integration assume the xerc20 address is the same in mainnet and blast

 function bridgeTo(
        address _to,
        address _erc20,
        address _remoteToken,
        uint256 _amount,
        uint32 _minGasLimit,
        bytes calldata _extraData
    ) external {
        // Sanity check
        if (_amount <= 0) {
            revert AmountLessThanZero();
        }

        address xerc20 = IXERC20Registry(registry).getXERC20(_erc20);
        address lockbox = IXERC20Registry(registry).getLockbox(xerc20);

        // Sanity check
        if (xerc20 == address(0) || lockbox == address(0)) {
            revert InvalidAddress();
        }

        // If using xERC20, the assumption is that the contract should be deployed at same address
        // on both networks.
        if (xerc20 != _remoteToken) {
            revert InvalidRemoteToken(_remoteToken);
        }

        SafeERC20.safeTransferFrom(IERC20(_erc20), msg.sender, address(this), _amount);
        SafeERC20.safeApprove(IERC20(_erc20), lockbox, _amount);
        IXERC20Lockbox(lockbox).deposit(_amount);
        SafeERC20.safeApprove(IERC20(xerc20), blastStandardBridge, _amount);
        L1StandardBridge(blastStandardBridge).bridgeERC20To(
            xerc20,
            _remoteToken,
            _to,
            _amount,
            _minGasLimit,
            _extraData
        );
    }

assume chain A has a deployed xerc20 contract.

the xerc20 contract in chain B is not deployed yet.

and the chain A's proxy admin is a smart contract wallet

an attacker may deploy the smart contract wallet with same address in chain B controlled by attacker

while the smart contract wallet address controlled by user Alice is in chain A

and deploy the xerc20 token in chain B,

as shown in https://rekt.news/wintermute-rekt/

in the wintermute case,

the smart contract in ethereum is created by CREATE Opcode

and the hacker brute force the nonce and deploy and control the same address in optimism network to claim the 20 OP tokekn,

showing that the same smart contract address in different network may controlled by different person.

user then bridge the xerc20 token from chain A to chain B and once chain B's xerc20 token receives fund.

the malicious proxy admin in chain B may upgrade the contract to steal all fund.

Tools Used

Manual Review

Recommended Mitigation Steps

one of the way to prevent this issue is checking that if the proxy admin address has code size (check if proxy admin is a smart contract) when deploying xerc20 token.

Assessed type

Token-Transfer

[jatinj615]

Protocol users DefaultProxyAdmin from OpenZeppelin while deploying on any chain.

[alcueca]

This is quite the chain of events to get to an exploit.

[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

[JeffCX]

Hello, I think this issue severity should be medium if not high

code-423n4/2023-09-maia-findings#877

the past same issue is judged as a high severity,

This is quite the chain of events to get to an exploit.

the only pre-conditioin is that the proxy admin is a smart contract address

and the blast adapter enforce the proxy admin is the same address in different network but the proxy admin can be controlled by different party.

[c4-judge]

This previously downgraded issue has been upgraded by alcueca

[c4-judge]

alcueca marked the issue as primary issue

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

You are right, I misjudged the preconditions, which are not so many, actually.

[alcueca]

In terms of mitigation, since proxyAdmin is a trusted account, it should be so that the same address is owned by the same entity in all chains (i.e., not a gnosis safe). If that is not possible, xERC20 should be considered untrusted until a trusted account (not proxyAdmin) validates them.

[3docSec]

Hi @alcueca,

I believe this finding did not deserve its upgrade.

The finding suggests that the same xERC20 address is being used categorically across different chains, while:

• this is only the case for Blast, as seen in the LockboxAdapterBlast
• other chains are bridged using Connext, which allows independently configuring the xERC20 address as detailed here

Furthermore, the finding assumes that the _proxyAdmin address is used to derive the deployment address, which is not the case as _deployERC20() uses CREATE3, which derives the address solely from the address of the deploying contract + salt.

In order for the xERC20 contract on Blast to fall under the control of the attacker, the protocol admins would have to:

• either:
  • deploy the xERC20Factory on L1 using an address that can be obtained by the attacker, which seems unlikely as the deployer address in use by the Renzo Protocol is an EOA, or
  • deploy the xERC20Factory on Blast to the same address as on mainnet, however without deploying the xERC20 in question with it, and having deployed the xERC20 contract on L1 from a msg.sender that can be obtained by the attacker and used to frontrun the deployment, then
• deploy and initialize the LockboxAdapterBlast without first deploying the xERC20 contract on Blast (to the same address as on mainnet) themselves, which would be a clear admin mistake

This is far from what the issue describes, and would be invalid as a finding as it would constitute a string of reckless admin mistakes.

[JeffCX]

this is only the case for Blast, as seen in the LockboxAdapterBlast

this is true, as show in original report, only blast adapter enforce the address must be the same in l1 and l2.

Furthermore, the finding assumes that the _proxyAdmin address is used to derive the deployment address, which is not the case as _deployERC20() uses CREATE3, which derives the address solely from the address of the deploying contract + salt.

the msg.sender that create the token is used to derive the salt and I feel like it is reasonable to assume there will be some deployment that set msg.sender and upgrade admin (that is not an error, it makes natural sense.)

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/xERC20/contracts/XERC20Factory.sol#L147

    function _deployXERC20(
        string memory _name,
        string memory _symbol,
        uint256[] memory _minterLimits,
        uint256[] memory _burnerLimits,
        address[] memory _bridges,
        address _proxyAdmin
    ) internal returns (address _xerc20) {
        uint256 _bridgesLength = _bridges.length;
        if (_minterLimits.length != _bridgesLength || _burnerLimits.length != _bridgesLength) {
            revert IXERC20Factory_InvalidLength();
        }
 @       bytes32 _salt = keccak256(abi.encodePacked(_name, _symbol, msg.sender));

        // Initialize function - sent as 3rd argument to the proxy constructor
        bytes memory initializeBytecode = abi.encodeCall(
            XERC20.initialize,
            (_name, _symbol, address(this))
        );

  @      bytes memory _creation = type(TransparentUpgradeableProxy).creationCode;

        // Constructor in Proxy takes (logic, admin, data)
        bytes memory _bytecode = abi.encodePacked(
            _creation,
            abi.encode(xerc20Implementation, _proxyAdmin, initializeBytecode)
        );

then in this case, control proxy admin in l2 / l1 is the same address control msg.sender -> control the cross-chain token address.

In order for the xERC20 contract on Blast to fall under the control of the attacker, the protocol admins would have to:

anyone can use factory to deploy xERC20 token, not only protocol, so using genosis safe wallet as msg.sender / proxy admin address is not really a admin error.

deploy the xERC20Factory on L1 using an address that can be obtained by the attacker,

deploy the xERC20Factory on Blast to the same address as on mainnet, however without deploying the xERC20 in question with it, and having deployed the xERC20 contract on L1 from a msg.sender that can be obtained by the attacker and used to frontrun the deployment

my original report has this information.

as shown in https://rekt.news/wintermute-rekt/

in the wintermute case,

the smart contract in ethereum is created by CREATE Opcode

and the hacker brute force the nonce and deploy and control the same address in optimism network to claim the 20 OP tokekn,

showing that the same smart contract address in different network may controlled by different person.

user then bridge the xerc20 token from chain A to chain B and once chain B's xerc20 token receives fund.

the malicious proxy admin in chain B may upgrade the contract to steal all fund.

thanks for judging to upgrade it to medium

beceause a medium issue only has the setting if xxxx, fund is lost,

also when user is calling the bridgeTo via the blast adapter, they are not really aware that proxy admin / msg.sender maybe malicious and controlled by different party in different network, which is a really dangerous. because when there are aware, fund are bridged and lost.

so let us not over-complicated things:

if a msg.sender / proxy admin does not controlled by the same party in different network

yet the blast adapter still enforce token address in l1 / l2 are the same, user fund are lost.

also this issue (smart contract address's owner can be different in different network) has been historically judged as medium.

code-423n4/2023-09-maia-findings#877

code-423n4/2023-09-ondo-findings#406

in the finding above, smart contract that bridge fund are lost

in this finding, all user found lost, if msg.sender / proxy controlled by different party in other network (anyone can deploy token and set proxy admin address)

finally,

this similar issue nearly cause 20M OP token damage.

https://rekt.news/wintermute-rekt/

so Thanks judge for applying fair judgement to make it a medium severity issue.

[3docSec]

Hi, just one clarification:

using genosis safe wallet as msg.sender / proxy admin address is not really a admin error.

true, but using a Gnosis safe that was created using an insecure factory that has been deprecated years ago is an admin error, and a big one.

Outside this specific flawed way of creating Gnosis safes, which unfortunately was used by Wintermute folks, wallets are not vulnerable to this issue. Maia DAO's Ulysses was a different case because the addresses in question were those of end users who are presumably more naive than Renzo admins.

This said, I don't want to engage in a discussion, so I won't reply if I disagree with following replies & let the judge do their work.

[JeffCX]

but using a Gnosis safe that was created using an insecure factory that has been deprecated years ago is an admin error, and a big one.

https://rekt.news/wintermute-rekt/

from this post, we can see that the wallet of wintermute that get exploited.

this is their multisig

https://etherscan.io/address/0x4f3a120e72c76c22ae802d129f599bfdbc31cb81

and etherscan nicely give us the transaction of the mutlsig wallet deployment

https://etherscan.io/tx/0xd705178d68551a6a6f65ca74363264b32150857a26dd62c27f3f96b8ec69ca01

the safe proxy factory v.1.1.1 is used

https://etherscan.io/address/0x76e2cfc1f5fa8f6a5b3fc4c8f4788f0116861f9b

the contract is not really deprecated and there are user actively deploy the contract.

Maia DAO's Ulysses was a different case because the addresses in question were those of end users who are presumably more naive than Renzo admins.

in renzo setting, any user (using EOA account or smart contract) can deploy the xERC20 token as msg.sender / proxy admin.

also enforcing msg.sender / proxy admin is the same cross-chain has issue,

consider the case, there is a smart contract that deploy a token in l1, but the same smart contract does not exist in l2, so no bridgeTo can happen, the impact is less severe, but anyway

smart contract deploying xerc20 token will leads to issue in xerc20 cross-chain flow.

yeah will leave the argument for judge to review, but thanks for the comments, I think overall this is a high-quality discussion/

👍

[alcueca]

Reviewing the CREATE3 implementation, this is true:

Furthermore, the finding assumes that the _proxyAdmin address is used to derive the deployment address, which is not the case as _deployERC20() uses CREATE3, which derives the address solely from the address of the deploying contract + salt.

All considered, I don't think the warden has shown conclusive proof of how this would actually be exploited and the impact. One requirement for this exploit to happen is for an innocent user to use a Gnosis Safe to deploy an xERC20, which is not the most common of events. On these grounds, I'm donwgrading this finding to QA again.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[JeffCX]

One requirement for this exploit to happen is for an innocent user to use a Gnosis Safe to deploy an xERC20,

user using any smart contract to deploy xERC20 token leads to issue.

showing that the same smart contract address in different network may controlled by different person.

user then bridge the xerc20 token from chain A to chain B and once chain B's xerc20 token receives fund.

the malicious proxy admin in chain B may upgrade the contract to steal all fund.

In summary:

if a smart contract in l1 or l2 deploy xERC20 token, and the smart contract does not control the same smart contract in the other l1 or l2, then no xERC20 can be bridged.

If a smart contract in l1 or l2 deploy xERC20 token, and the other party control the same smart contract in other l1 or l2, then user fund are lost after calling bridgeTo because the blast adapter enforce xERC20 token address must be the same in l1 and l2.

the report also needs to reply on the assumption that msg.sender == proxy admin, controlling msg.sender only make sure the same token address can be deployed

only controlling proxy admin leads to loss of fund as highlighted in the original report, which is the impact I want to highlight.

also code-423n4/2024-04-renzo-validation#770 (comment)

is a separate issue from this one.

[c4-judge]

alcueca marked the issue as not selected for report