Full Smart Contract Upgradeability Discussion

hi @Gandharv_dalal

If you want to start the truffle console you need to start ganache first .
Ganache is your local blockchain emulation, truffle will try to get connected to a blockchain to interact with it. If it can’t find one you will get this error.

You can also start truffle this way.

truffle development

Truffle will start his own local blockchain and it ll deploy your contract locally by typing deploy.

But all this notions are explain in the eth 101 and 201 course so i think you need to get back to those course before continuing the smart contract security course which is a most advance course

1 Like

Hi @gabba,

if I correctly understand this adjustment does it then make the initialize function obsolete in the DogsUpdated contract as we have already set the Proxy owner variable in the constructor?

Hi @Filipo24

You need to be the owner of the proxy contract to upgrade the address of the functional contract.
But when you are upgrading it you can still change the owner saved in the storage by calling the initialize function.
I m not sure it really make sense with the current contract, but you can use this functionality to change the ownership of the new contract version

2 Likes

@filip I see that the upgrade function is callable by anybody. Why not make this function onlyOwner? because this could be potentially very dangerous. I would make the proxy.sol look like this:

pragma solidity 0.5.12;

import "./Storage.sol";

contract Proxy is Storage {

  address currentAddress;

  constructor(address _currentAddress) public {
    currentAddress = _currentAddress;
    owner = msg.sender;
  }

  modifier onlyOwner {
    require(msg.sender == owner);
    _;
  }

  // upgrade address of the functionality contract
  function upgrade(address _newAddress) public onlyOwner {
    currentAddress = _newAddress;
  }

  // Fallback function that is used to proxy to our functionality contract
  function () payable external {
    //REDIRECT to currentAddress
    address implementation = currentAddress;
    require(currentAddress != address(0));
    bytes memory data = msg.data;

    // Delegatecall every function call to the underlying functional contract
    // making this contract a proxy contract
    assembly {
      let result := delegatecall(gas, implementation, add(data, 0x20), mload(data), 0, 0)
      let size := returndatasize
      let ptr := mload(0x40)
      returndatacopy(ptr, 0, size)
      switch result
      case 0 {revert(ptr, size)}
      default {return(ptr, size)}
    }
  }
}

@filip in your DogsUpgdated contract which inherits from Dogs you skip the fact that the setNumberOfDogs function also got onlyOwner added to it during the course. Is it possible to modify an existing function in an inherited contract? Does it overrule the parent function completely? Or is it to be seen as an overloaded function (even though the arguments are the same except a onlyOwner modifier has been added)

@Dhardesh
Yes, you are right, the upgrade function should not be public. One way is to add a modifier as you mentioned.
Filip has already added a validation that msg.sender == owner as shown in the pic below hence no need to add modifier then. :slight_smile:

Yes, we can override the function of the parent contract to the inherited contract.

Depends, on which contract you are calling. If you are calling the parent contract then it will not override.
And if you are calling the inherited contract it will override completely.

It is overriding function and not overloading.

Thanks

Hi,

i have basically the same question concerning this owner issue. Wouldnt it be much easier to simply set the owner in the constructor of the proxy contract?

Best,
Philipp

HELLO @Taha @filip @gabba
Screenshot 2020-09-30 at 10.30.44 AM

Screenshot 2020-09-30 at 10.33.33 AM

Why is it that we need to do proxyDog.initialize manually (line 33) here. Doesn’t it get initialize when it is being deployed (line 27) by the constructor (line 12) ?

Does the constructor function runs when the instance of DogsUpdates is passed to proxy.address (line 31)?

2 Likes

Hey @lusivekrane

Because you are using delegate call, the modifier onlyOwner checks the variable owner in the proxy contract, therefore even if you use a constructor in DogsUpdated, the owner variable will not be initialised in you proxy but only in DogsUpdated.
I know it might be confusing the first time that you work with proxies :slight_smile:
I strongly advise you to read this from OpenZeppelin, it is an amazingly written article about proxies.

https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies

You should read it all, but if you just want to jump to the constructor caveat read this part:

https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies#the-constructor-caveat

Also take your time to run some unit testing using Truffle, play with your variables and inspect them.

What you are learning now it’s really important and will take some time to sink in, don’t rush and keep going :slight_smile:

Happy learning,
Dani

3 Likes

@dan-i I have the same doubt, and I will read the article about proxies that you mention in OpenZeppelin, but I disagree when you say that the owner varable will not be initialized in proxy but only in dogsUpdated, since in the proxy contract you also have a constructor:

1 Like

hey @javier_ortiz_mir

You are right when you say that the proxy has a constructor, also your smart contract (logic contract) can have it’s own constructor, but it will not be executed in the context of the proxy’s state.
When a proxy is involved, it’s ofter used also an Initializable contract or function. This is a function that is executed only once in it’s lifetime, and will be used as a constructor.

Happy to see that you are diving deep into the topic.

Happy learning,
Dani

1 Like

Thanks for the reply @dan-i , it really makes the learning much more interactive! But I’m afraid I do NOT understand when you say that “it will not be executed in the context of the proxy state” what do you mean by that?

My understanding of the constructor function is that it only executes once, and it executes when the contract is created. And my understanding of the truffle environment (this is also my first time using truffle, and I haven’t used ganache by the way) is that when you do migrate, you do deploy the contracts that were previously compiled.

Hey @javier_ortiz_mir

My big suggestion for you is indeed to test it.
Use truffle test to try it out and inspect the variables from the proxy contract and from the contract instance.
You will see different values.
I had to do the same myself when I was studying it :slight_smile:

cheers,
Dani

1 Like

I understand that we add a bunch of mappings in storage.sol, to be flexible with basic variable types in the future, but what if we need additional variables of type mapping moving forward? Is there a way to implement that in a simple way?

Theoretically I should be able to do something like this

mapping (string => mapping (string => uint256)) _uintMappingStorage;
//declared and set like this: _uintMappingStorage["newMapping"]["numberDogs"] = 5;

But then I would have to do that for each combination of basic variables (25 possibilities).

The code:

  function initialize(address _owner) public {
    require(!_initialized);
    owner = _owner;
    _initialized = true;
  }

It looks like it is not exposed to the re-entrancy attack since it does not call any external function and specifically it does not call any external address

1 Like

I had the same issue on linux. I have solved upgrading truffle.

@filip I have a question:

Why did we not restrict the upgrade() function inside Proxy to the owner only? This would mean an attacker can call this function to point to any other contract, they can copy the structure of Storage and make a malicious Dog contract and set themselves as the owner in initialize via delegate call.

or am I completely missing something? :confused:

2 Likes

@Jesal_Patel

It should be restricted to the owner indeed :slight_smile:

2 Likes

How to handle events?

My presumption is:

  • declare events in functional contract
  • events are automatically forwarded to the right contract address(proxy)
  • event listener can be set up with web3.js to listen at proxy
proxyDog.events.someEvent().on(some actions...);

Is this correct?