Upgradeable ContractA that calls ContractB that has a delegatecall. Is it really unsecure?

Support Upgrades
1

Hello, when I try to deploy this code with upgrades.deployProxy it is raising the following error:

Error: Contract `contracts/Contracts.sol:ContractA` is not upgrade safe

contracts/Contracts.sol:65: Use of delegatecall is not allowed
    https://zpl.in/upgrades/error-002

contracts/Contracts.sol:65: Use of delegatecall is not allowed
    https://zpl.in/upgrades/error-002

I know I can ignore it by passing the unsafeAllow: ["delegatecall"] parameter, but I wanted to double check if this code is really unsecure or not, because from what I understand, the call from ContractA to ContractB is not a delegatecall and even being it, it will need to check for the role. Even if we execute ContracA's implementation contract directly, it will need to check the role anyway (with implementation's storage but won't work anyway). Am I missing something?

Thanks

PS: I see that adding /// @custom:oz-upgrades-unsafe-allow-reachable delegatecall NatSpec to to ContractB's callContractC also works for ignoring. Should be the way to go?

:1234: Code to reproduce

pragma solidity ^0.8.20;

import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

contract ContractA is Initializable, AccessControlUpgradeable, UUPSUpgradeable {

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize() public initializer {
        __AccessControl_init();
        __UUPSUpgradeable_init();
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
    }

    function _authorizeUpgrade(address newImplementation) internal override onlyRole(DEFAULT_ADMIN_ROLE) {}

    function callContractC(
        address contractC,
        address contractB,
        uint256 amount
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        ContractB(contractB).callContractC(contractC, amount);
    }
}

contract ContractB {
    address private immutable contractA;

    constructor(address contractA_) {
        contractA = contractA_;
    }

    function callContractC(address contractC, uint256 amount) external {
        (amount);
        require(contractA == msg.sender, "Only contractA can call");
        (bool success, ) = contractC.delegatecall(msg.data);
        require(success, "Delegate call to ContractC failed");
    }
}

contract ContracC {
    function callContractC(address contractC, uint256 amount) external pure {
        (contractC);
        (amount);
    }
}

:computer: Environment

  • hardhat 2.22.3
  • @openzeppelin/hardhat-upgrades 3.1.0
  • @openzeppelin/contracts-upgradeable 5.0.2
2

Sorry if I asked this in the wrong place on the forums. Should I move it to another place?

Thanks

1 Like
3

I usually restrict with a modifier the calls between contracts to the only contract in question that can be called. That is to say, if my contracts call each other I restrict the calls to their addresses for security.

4

@Cainuriel that is what I'm doing in the example, isn't it?

5

Yes, that's what you do in the callContractC function.

But in it you call a contract C that you pass by parameter from a contract B.

It is confusing to me what you intend to do:

Your contract C seems to call itself with its function callContracC.

Your contract B does check that it is contract A that calls callContractC.

Your contract A receives contracts in parameters to be called, contract B and contract C ,but to me this is not possible named contract C because C can be any contract.

I find it confusing and I don't quite understand what you are trying to do.

In my case, I have some functions that can only be called by the contract that I indicate. This contract is set or initialized as a global variable and I never pass it as a parameter. But in your example you have a wiggle of contracts that I find it difficult to understand its functionality.

Especially because you can pass any contract by parameter, except in your contractB that if it has in global the contract A. It is the situation that is more similar to what I do.

1 Like