Research question regarding Proxy.sol

[webthethird]

Hello!

This question is meant primarily for @zyzek, since the version of Proxy.sol I am looking at is attributed to him, though I'm happy to take answers from anyone else who contributed to the contract in question.

Brief background: I am a graduate student and smart contract security researcher at the Illinois Institute of Technology in Chicago. My current research is focused specifically on upgradeable proxy contracts, and I've come across this proxy from Synthetix several times in the data set we're using in our analysis. I've been using a modified version of Slither to extract proxy features related to security and safety concerns.

The feature which this question is about is the presence or absence of compatibility checks when updating the target address. Below are a few examples of compatibility checks from other proxy patterns, to give you a sense of what we're looking for.

// From an EIP-1967 proxy:
require(OpenZeppelinUpgradesAddress.isContract(newImplementation), 
        "Cannot set a proxy implementation to a non-contract address");

// From an EIP-1822 (UUPS) proxy:
require(
        bytes32(
            0xc5f16f0fcc639fa48a6947836d9850f504798523bf8c9a3a87d5876cf622bcf7
        ) == Proxiable(newAddress).proxiableUUID(),
        "Not compatible"
    );

Now, when looking at the Synthetix proxy, I initially wasn't sure if the setTarget function below should be flagged as having a compatibility check, due to its signature which specifies a contract type for the _target parameter:

function setTarget(Proxyable _target)
    external
    onlyOwner
{
    target = _target;
    emit TargetUpdated(_target);
}

I wasn't sure if the EVM would perform any type checking on the argument passed in, though I did search the Solidity language docs and learned that the ABI for this function would be converted to setTarget(address). So I did a quick experiment by deploying the proxy contract to a local VM using Remix, and discovered that I could in fact set the target to a non-contract address, meaning there is no type checking.

So, my question is: was this contract developed under the assumption that only a contract inheriting Proxyable could be passed into setTarget(Proxyable _target) without triggering an execution error? In other words, is this a failed compatibility check? Or did you already know the EVM would not perform type checking, and just didn't feel the need to confirm that it is in fact compatible (i.e., missing a check rather than checking incorrectly)?

Of course, this target setter is protected with an onlyOwner modifier, so it's not unreasonable to assume that the owner knows better than to set the target to an incompatible address. But my professor said to operate under the assumption that this was intended to be a compatibility check but fails to do so, and I want to make sure we're not reading too much into this function signature.

Thank you for your time!
William E Bodell III
WEBthe3rd.eth

P.s. Anton, we have a few connections in common! Way back in 2017, I joined a hackathon team at Consensus in NYC with Tim Bass and Sam Brooks, along with Lucas Cullen and Nick Addison. My new Aussie friends and I ended up winning the hackathon and taking home two additional prizes. If you're still in touch with them, let them know that Bill with the Ethereum tattoo says hi!

[zyzek]

Hi William,

Thanks for reaching out, and apologies for the delay in replying, I hope it's not too late to be useful.

We were aware at the time that the Proxyable type would ultimately just stand in for address and no runtime check would be performed.

In fact I'm sure we consciously used this lack of checking on more than one occasion (e.g. deactivating a contract by pointing the proxy to null). I guess it felt like good practice to enforce the type at the Solidity level, and also useful to keep in mind conceptually, given that our proxy architecture was a bit non-standard, being based on CALL rather than on DELEGATECALL, and hence required the proxy to manipulate its underlying contract through a specific functional interface.

I note that this style of proxy, slightly bizarre as it has turned out to be, was authored back in the foggy mists of time before there was much in the way of information around about proxies, and maybe even before view functions commonly used the STATICCALL opcode. It was structured in this way so that implementations could be swapped very easily without making any assumptions about storage architecture, which was quite convenient. But both Synthetix and the space in general have moved on given its limitations, and the next major version of the system will be moving to a new architecture, which is currently under development here: https://github.com/Synthetixio/synthetix-v3/tree/main/packages/core-contracts/contracts/proxy.

Fun to hear about the connection with that Consensus hackathon crew - I remember well hearing Tim and co talk nostalgically about it. Unfortunately I haven't spoken to them in some time, but I'll try to remember you if I run into them around the traps.

Hope this helps,

Anton