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

Malicious proxyAdmin can upgrade xerc20 token implementation and steal fund from user. #770

Closed
c4-bot-9 opened this issue May 8, 2024 · 5 comments
Assignees
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality 🤖_primary AI based primary recommendation 🤖_174_group AI based duplicate group recommendation

Comments

@c4-bot-9
Copy link
Contributor

c4-bot-9 commented May 8, 2024

Lines of code

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

Vulnerability details

Impact

Malicious proxyAdmin can upgrade xerc20 token implementation and steal fund from user.

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);

the lock box contract enforce that when withdraw, the token must be burnt,

but Malicious proxyAdmin can upgrade lock box implementation and steal fund directly (one of the way is minting a lot of tokens).

consider any user can deploy xerc20 token and lock box contract, the proxy admin can be anyone and may not be trusted.

Tools Used

Manual Review

Recommended Mitigation Steps

N/A

Assessed type

Token-Transfer

@c4-bot-9 c4-bot-9 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 8, 2024
c4-bot-1 added a commit that referenced this issue May 8, 2024
@c4-bot-12 c4-bot-12 added 🤖_174_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels May 8, 2024
@DadeKuma
Copy link

proxyAdmin is a trusted role

@DadeKuma
Copy link

@howlbot reject

@howlbot-integration howlbot-integration bot added the insufficient quality report This report is not of sufficient quality label May 10, 2024
@JeffCX
Copy link

JeffCX commented May 25, 2024

Anyone can permissionlessly deploy xERC20 token and set a proxy admin contract so proxyAdmin is not always trusted role

and in reality when user interact with the xERC20 token token, they cannot know whether the proxy admin is maliciously are not and proxy admin does retain the power the upgrade the contract,

simllar finding are judged as medium consistently in the past (if the deployment is permissionless, malicious deployer has a way to steal fund from user.)

code-423n4/2023-07-pooltogether-findings#300

code-423n4/2023-01-popcorn-findings#552

I think this should be a fair medium risk issue.

@alcueca
Copy link

alcueca commented May 28, 2024

This would be a duplicate of code-423n4/2024-04-renzo-findings#503, which has been downgraded to QA

@JeffCX
Copy link

JeffCX commented May 28, 2024

Thanks for reviewing my report.

This report's main concern is user interact with xERC20 deployed by permissionlessly malicious proxy admin.

issue #503 concerns the issue and impact if a smart contract deploys a xERC20 token in one network but the same contract does not control same address in the other network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality 🤖_primary AI based primary recommendation 🤖_174_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

5 participants