TestRegistry.sol does not serve as a good testing example

[barakman]

In this contract:

pragma solidity ^0.4.18;

// This contract is only used for testing purposes.
contract TestRegistry {

  mapping(address => uint) public registry;

  function register(uint x) payable public {
    registry[msg.sender] = x;
  }

}

What exactly is stopping me from calling function register directly???

Isn't the purpose of Multi-Sig to restrict permission further than onlyOwner?

In this example, the function is not restricted even to onlyOwner!

I believe that it would be advisable to:

1. Import Ownable.sol
2. Inherit from Ownable
3. Declare function register as onlyOwner

Then, in simplemultisig.js, after the line

let reg = await TestRegistry.new({from: accounts[0]})

Add the line:

await reg.transferOwnership(multisig.address);

Side-note - during execution, an exception is thrown at:

await multisig.execute(sigs.sigV, sigs.sigR, sigs.sigS, reg.address, value, data, {from: accounts[0], gasLimit: 1000000})

So I have yet to figure out the entire process, but I nevertheless believe that this is the type of capability which should be demonstrated here (i.e., calling a previously-only-owner function via the MultiSig contract).

Update on the side-note above:

The error was thrown because I removed the payable keyword from the declaration of function register.

After "re-enforcing" it, the test runs to completion.

[coder5876]

I don't understand why you would want all this extra complexity? We are simply testing if we can successfully call a function of a smart contract via the multisig and have the function be executed correctly. Maybe it would be good to have a test with of a function with multiple inputs of different types but I don't see where you are going with your question.

[barakman]

@christianlundkvist: Thank you for the quick response. Kindly explain the point in restricting a function via MultiSig, when anyone (and everyone) can call it directly.

[coder5876]

@barakman The point is not to restrict a function via MultiSig, the point is just to test calling any function of any smart contract. Would you have the same objections if this was an ERC20 token contract for instance (anyone can call the transfer function there too)?

[barakman]

@christianlundkvist: I don't quite understand the point of "just to test calling any function of any smart contract", since this is something that one can perform without any additional infrastructure.

[devrandom]

@barakman TestRegistry does actually help test what we want to test here:

• it helps test that Simple Multisig can call a method with arbitrary parameters
• it helps test that the call is made with the multisig contract as sender. I'm not sure this is actually validated in the existing tests, but perhaps it's not worth testing since this is a known property of the EVM.

As a side benefit, it actually not true that TestRegistry has no security. A sender can only modify the registry entry keyed by the sender contract ID. You cannot modify arbitrary entries that you didn't create. However, this is incidental to what we want to test, because enforcing authorization based on the sender contract ID is a concern of the target contract, and is therefore out of scope of simple-multisig. That said, perhaps it would be good to document the security assumptions on the target contract. ERC20, for example, fulfills those assumptions.