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

compilationTarget not check will cause security dangerous #1071

Closed
lolieatapple opened this issue Jun 20, 2023 · 16 comments
Closed

compilationTarget not check will cause security dangerous #1071

lolieatapple opened this issue Jun 20, 2023 · 16 comments
Assignees

Comments

@lolieatapple
Copy link
Contributor

lolieatapple commented Jun 20, 2023

image

compilationTarget not check will cause security dangerous.

My contract contains multiple files, one of which can pass verification but is not a compilationTarget. It disguises its code and poses a risk that hackers could use an upgradable contract to mimic an un-upgradable one, leading to security risks.

View in Huly HI-424

@marcocastignoli
Copy link
Member

I tried to replicate the issue, but couldn't make it, maybe I followed the wrong steps. Can you please provide more details?

@lolieatapple
Copy link
Contributor Author

lolieatapple commented Jun 21, 2023

ok.
First, you write a demo upgradeable smart contract such as:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"

contract MockToken is ERC20 {
    constructor() ERC20("MockToken", "MT") {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }
}

and then, compile and deploy the MockToken contract first:
address: AAA

after, deploy the TransparentProxy contract with the pre MockToken as the implement logic.
address: BBB

Finally, you can verify MockToken contract use proxy address BBB because its source file includes ProxyAdmin.sol and ProxyAdmin.sol includes TransparentProxy source file.

I think you compared all the bytecodes in compile result not the exactly MockToken bytecodes.

such as:

File: Abc.sol
has both contracts:
contract A {}
contract B {}

you can success verified when you try to use File Abc.sol to verify both contract A and contract B.

And you can verify contract A with Abc.sol for B's address. (This is the issue).

@marcocastignoli
Copy link
Member

Before actually trying this I should say that Sourcify doesn't support proxies. You cannot use the MockToken source code to verify the proxy contract and vice versa.

Or in other words, the intended behavior is that: you can verify the proxy contract only by using the proxy source code, and you can verify the MockToken contract only by using the MockToken source code.

@lolieatapple
Copy link
Contributor Author

Yes, you are right. However, the reality is that it has mistakenly provided support.

@marcocastignoli
Copy link
Member

Sorry but I'm still having problems replicating the vulnerability, I hope you can understand :)

This is what I did:

  • compile and deploy MockToken
  • compile and deploy proxy with MockToken as the implement logic
  • using sourcify UI:
    • I uploaded the metadata of the proxy, the source code of the proxy. I used the address of the MockToken contract. Verification failed with: The deployed and recompiled bytecode don't match.
    • I uploaded the metadata of MockToken, the source code of MockToken. I used the address of the Proxy contract. Verification failed with: The deployed and recompiled bytecode don't match.

I think I'm doing something wrong, proably it's the contract I'm using. Can you please provide an environment in which I can test the problem? Thanks again for the support.

@lolieatapple
Copy link
Contributor Author

the source of the proxy is the same source with MockToken.sol. is it?

@lolieatapple
Copy link
Contributor Author

and which proxy do you use? is it the TransparentProxy in the proxyAdmin?

@lolieatapple
Copy link
Contributor Author

and have you see this?

such as:

File: Abc.sol
has both contracts:
contract A {}
contract B {}

you can success verified when you try to use File Abc.sol to verify both contract A and contract B.

And you can verify contract A with Abc.sol for B's address. (This is the issue).

@lolieatapple
Copy link
Contributor Author

I can give you my screen snap about it, please wait 5 minutes.

@marcocastignoli
Copy link
Member

the source of the proxy is the same source with MockToken.sol. is it?

yes

and which proxy do you use? is it the TransparentProxy in the proxyAdmin?

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";


contract TransparentUpgradeableProxy is ERC1967Proxy {
    // implementation

and have you see this?

maybe that's the part I skipped because I didn't get it

@lolieatapple
Copy link
Contributor Author

This is my test code:
image
First, deploy the MockDemo contract.
image
Second, switch to the TransparentUpgradeableProxy in the same sol file to deploy.
image
This is the deploy address:
image
Download verify files from here, and import them to sourcify.dev
image
and verify the proxy.
image
and verify the mockDemo.
image
Important step, verify mockDemo again with proxy address:
image
it shows successed.
and in explorer the source file is:
image
it shows the MockDemo source files in proxy address. and api return the same info.

I think the reason is same json file contains both proxy and mockDemo code in it:
image

this is my new demo source:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

contract MockDemo {
    address public owner;

    function setOwner(address to) public {
        owner = to;
    }
}

and this is the explorer URL, the explorer display info was come from sourcify api.
https://testnet.wanscan.org/address/0x55a03A27EfE92140c518b6ef677628983030F029

Thanks. please help ~~~

@marcocastignoli
Copy link
Member

Thanks a lot for the detailed description. I'm looking at it 🙏

@marcocastignoli
Copy link
Member

ok @lolieatapple I finally replicated it! We are on it

@lolieatapple
Copy link
Contributor Author

Ok, Great! Thanks!~~

marcocastignoli added a commit that referenced this issue Jun 21, 2023
 * fix missing check for address/chainId
@Hellobloc
Copy link

Hellobloc commented Jun 21, 2023

@lolieatapple Fantastic find, but I see that your browser only takes the contract name part of sourcify's compile target. I am concerned that this may lead to ambiguity, take a look at this issue.
The good news is that sourcify did a great job removing the extra source files which prevented us from building two source files with the same contract name, but there are still some special cases where extra source files are allowed, such as the point mentioned in this issue.

@marcocastignoli
Copy link
Member

marcocastignoli commented Jun 22, 2023

I tried again the process on staging.sourcify.dev and it seems solved. (The output is not clear but the proxy contract files are not being replcaced by the Mock contract sources)

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

No branches or pull requests

3 participants