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

Add reverse proxy call to owner #26

Closed
wants to merge 1 commit into from
Closed

Conversation

Amxx
Copy link

@Amxx Amxx commented Mar 5, 2019

owner might be a multisig, this allows read from/management of any eventual owning contract directly from the proxy. Usefull if the owning contract implements interfaces like ERC1271. That way dapps can call them without having to resolve the proxy ownership, thus making the proxy transparent

owner might be a multisig, this allows read from/management of any eventual owning contract directly from the proxy. Usefull if the owning contract implements interfaces like ERC1271. That way dapps can call them without having to resolve the proxy ownership, thus making the proxy transparent
@Amxx
Copy link
Author

Amxx commented Mar 5, 2019

Is that just implementation details or should I also propose an update to the ERC draft ?

@Amxx
Copy link
Author

Amxx commented Mar 6, 2019

The drawback of this proposal is that if a transaction is sent with the arbitrary information in the data (for reference tracking for example) and if the first bytes4 is not null, then the data will be interpreted as a contract call to the owner, which is likely to revert.

I believe this is a minor drawback compared to the advantages it offers, but users should consider it and prepend a null bytes4 to any data that isn't intended as a function call

@frozeman
Copy link
Contributor

Looks awesome, and if we add an address owner instead of owner on key 0x0, we can simplify that a lot.

What happens when somebody sends tokens or ETH to this contract? where would the ETH or tokens be stored? As it uses call i assume they would sit on the proxy, but let the owner react.

Most standard interfaces would not add a null byte manually, what exactly do you mean with that?

@frozeman
Copy link
Contributor

Here is an implementation with only the address:

pragma solidity ^0.5.4;

interface ERC725 {
    event DataChanged(bytes32 indexed key, bytes32 indexed value);
    event OwnerChanged(address indexed ownerAddress);
    event ContractCreated(address indexed contractAddress);

    function changeOwner(address _owner) external;
    function getData(bytes32 _key) external view returns (bytes32 _value);
    function setData(bytes32 _key, bytes32 _value) external;
    function execute(uint256 _operationType, address _to, uint256 _value, bytes calldata _data) external;
}


contract ProxyAccount is ERC725 {
    
    uint256 constant OPERATION_CALL = 0;
    uint256 constant OPERATION_CREATE = 1;

    mapping(bytes32 => bytes32) store;
    
    address public owner;
    
    constructor(address _owner) public {
        owner = _owner;
    }


    modifier onlyOwner() {
        require(msg.sender == owner, "only-owner-allowed");
        _;
    }
    
    // ----------------
    // Public functions
    
    function () external payable {}
    
    function changeOwner(address _owner)
        external
        onlyOwner
    {
        owner = _owner;
    }

    function getData(bytes32 _key)
        external
        view
        returns (bytes32 _value)
    {
        return store[_key];
    }

    function setData(bytes32 _key, bytes32 _value)
        external
        onlyOwner
    {
        store[_key] = _value;
        emit DataChanged(_key, _value);
    }

    function execute(uint256 _operationType, address _to, uint256 _value, bytes calldata _data)
        external
        onlyOwner
    {
        if (_operationType == OPERATION_CALL) {
            executeCall(_to, _value, _data);
        } else if (_operationType == OPERATION_CREATE) {
            address newContract = executeCreate(_data);
            emit ContractCreated(newContract);
        } else {
            // We don't want to spend users gas if parametar is wrong
            revert();
        }
    }

    // copied from GnosisSafe
    // https://github.com/gnosis/safe-contracts/blob/v0.0.2-alpha/contracts/base/Executor.sol
    function executeCall(address to, uint256 value, bytes memory data)
        internal
        returns (bool success)
    {
        // solium-disable-next-line security/no-inline-assembly
        assembly {
            success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0)
        }
    }

    // copied from GnosisSafe
    // https://github.com/gnosis/safe-contracts/blob/v0.0.2-alpha/contracts/base/Executor.sol
    function executeCreate(bytes memory data)
        internal
        returns (address newContract)
    {
        // solium-disable-next-line security/no-inline-assembly
        assembly {
            newContract := create(0, add(data, 0x20), mload(data))
        }
    }
}

@Amxx
Copy link
Author

Amxx commented Mar 11, 2019

What happens when somebody sends tokens or ETH to this contract? where would the ETH or tokens be stored? As it uses call i assume they would sit on the proxy, but let the owner react.

Eth would stay on the proxy (the call does not transfer any value). Tokens would be owned by the proxy. And yes, it would let the owner react

Most standard interfaces would not add a null byte manually, what exactly do you mean with that?

If i send 0.01Eth for a hot dog, and do a transaction to a proxy with "hot dog" in the data field, then the "hot dog" data would be interpret as 0x686f7420646f67 which would trigger a call on the owner.
If the owner is a wallet or a contract with an empty fallback function then everything will go well, but otherwise it will revert the whole transaction

@Amxx
Copy link
Author

Amxx commented Mar 11, 2019

Since I wrote this pull request I realized the issue with ERC725 proxy is also that the assets (eth and tokens) are not stored at the same address then the multisig, which makes it difficult for the multisig to refund the relayer of a meta transaction with eth, and even more with tokens.
This lead me to write a completely different proxy mechanism, that won me 2 prizes at EthParis. I invite you to read it here:
https://devpost.com/software/universally-upgradable-identity-proxy
prototype implementation is here:
https://github.com/Amxx/ERC1836UpgradableIdentityProxy/

@2075
Copy link
Member

2075 commented Mar 11, 2019

Excellent work and congrats

@frozeman
Copy link
Contributor

frozeman commented Jul 2, 2020

Very good addition, i will add it to the ERC725 Account implementation for now, but this should be also part of the standard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants