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

fix(forge): create decode constructor args #8998

Closed
wants to merge 1 commit into from

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Oct 1, 2024

Motivation

Closes #8995

when we populate the CREATE tx and attempt to decode args from constructor we assume that the constructor args start after bytecode len:

let constructor_args = &creation_code[bytecode.len()..];

In mentioned issue, for https://github.com/isle-labs/isle-contract/blob/fix-audit-issue/scripts/DeployGlobals.s.sol which deploys an uups proxy

contract UUPSProxy is ERC1967Proxy {
    constructor(address _implementation, bytes memory _data)
        ERC1967Proxy(_implementation, _data)
    {}
}

the sliced data contains extra (assume constructor sig) which results in

65676174652063616c6c206661696c6564
000000000000000000000000663f3ad617193148711d28f5334ee4ed07016602
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000000

Since the first constructor arg is address 663f3ad617193148711d28f5334ee4ed07016602, decoding input fails with "Failed to decode constructor arguments" at

let values = constructor.abi_decode_input(constructor_args, false).inspect_err(|_| {
error!(
contract=?self.contract_name,
signature=%format!("constructor({})", constructor.inputs.iter().map(|p| &p.ty).format(",")),
is_create2,
constructor_args=%hex::encode(constructor_args),
"Failed to decode constructor arguments",
);

Solution

  • proposed solution is to padd &creation_code[bytecode.len()..] and then remove first 32 bytes, e.g. in sample above:
65676174652063616c6c206661696c6564
000000000000000000000000663f3ad617193148711d28f5334ee4ed07016602
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000000

padded as

00000000000000000000000000000065676174652063616c6c206661696c6564
000000000000000000000000663f3ad617193148711d28f5334ee4ed07016602
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000000

and then resulting in

000000000000000000000000663f3ad617193148711d28f5334ee4ed07016602
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000000
  • wasn't able to find a minimal repro without using the uups / ERC1967 proxies

@klkvr
Copy link
Member

klkvr commented Oct 1, 2024

looks like the actual issue is that UUPSProxy is identified as ERC1967Proxy?

code:

function deployGlobals() internal broadcast(deployer) returns (IsleGlobals globals_) {
    globals_ = IsleGlobals(address(new UUPSProxy(address(new IsleGlobals()), "")));
    globals_.initialize(governor);
}

trace:

  [1442215] DeployGlobals::run()
    ├─ [0] VM::startBroadcast(0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC)
    │   └─ ← [Return] 
    ├─ [1248187] → new IsleGlobals@0x663F3ad617193148711d28f5334eE4Ed07016602
    │   └─ ← [Return] 6223 bytes of code
    ├─ [51501] → new ERC1967Proxy@0x2E983A1Ba5e8b38AAAeC4B440B9dDcFBf72E15d1
    │   ├─ emit Upgraded(implementation: IsleGlobals: [0x663F3ad617193148711d28f5334eE4Ed07016602])
    │   └─ ← [Return] 136 bytes of code
    ├─ [68824] ERC1967Proxy::initialize(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266)
    │   ├─ [68441] IsleGlobals::initialize(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266) [delegatecall]
    │   │   ├─ emit Initialized(governor_: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266)
    │   │   └─ ← [Stop] 
    │   └─ ← [Return] 
    ├─ [0] VM::stopBroadcast()
    │   └─ ← [Return] 
    └─ ← [Return] ERC1967Proxy: [0x2E983A1Ba5e8b38AAAeC4B440B9dDcFBf72E15d1]

error:

Failed to decode constructor arguments contract=Some("ERC1967Proxy") signature=constructor(address,bytes) is_create2=false constructor_args=65676174652063616c6c206661696c6564000000000000000000000000663f3ad617193148711d28f5334ee4ed0701660200000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000

and because ERC1967Proxy has smaller bytecode, we end up treating part of UUPSProxy bytecode as constructor args

@grandizzy
Copy link
Collaborator Author

looks like the actual issue is that UUPSProxy is identified as ERC1967Proxy?

Ah, thank you, yep, doesn't look right. Going to check why this happen

@grandizzy
Copy link
Collaborator Author

related to deploying with cbor_metadata set to false, no code change required

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

Successfully merging this pull request may close these issues.

bug(forge script): Failed to decode constructor arguments: buffer overrun while deserializing
2 participants