Safe transaction service reporting incorrect signer when using ledger with `proposeTransactionsWithSignature()`

[zacshowa]

Background

I've been using safe-utils to write a solidity script that involves batching a few transactions and proposing them to a safe wallet like so:

function addToBatch(address target, bytes memory callData) internal {
        batchTargets.push(target);
        batchData.push(callData);
}


function proposeBatchTransaction() internal returns bytes32{
        //Generate transaction target and data arrays for signing.
        bytes memory transactionDataContract1 = abi.encodeCall(ownable2StepContract1.acceptOwnership, ());
        bytes memory transactionDataContract2 = abi.encodeCall(ownable2StepContract2.acceptOwnership, ());
        bytes memory transactionDataContract3 = abi.encodeCall(ownable2StepContract3.acceptOwnership, ());

        addToBatch(address(contract1), transactionDataContract1);
        addToBatch(address(contract2), transactionDataContract2);
        addToBatch(address(contract3), transactionDataContract3);

        //Get composed transaction data
        (address to, bytes memory data) = safe.getProposeTransactionsTargetAndData(batchTargets, batchData);

        // Get signature over transaction so we can propose it to the multisig wallet.

        bytes memory signature = safe.sign(to, data, Enum.Operation.Call, proposerAddress, derivationPath);

        //Use signature to propose transaction to the safe wallet
        bytes32 txHash = safe.proposeTransactionsWithSignature(
            batchTargets,
            batchData,
            proposerAddress,
            signature
        );
        return txHash;
}

The script uses several contract level variables. Namely, batchTargets, batchData, proposerAddress, and derivationPath.
The types associated with these are as follows.

    address[] internal batchTargets;
    bytes[] internal batchData;
    // The following are read in via vm.env* cheatcodes.
    address proposerAddress; // This will be the address of the account stored in the ledger at derivationPath
    string derivationPath;

Issue

From my understanding this should mean while running the script, a user would be using their ledger wallet to sign this batch of transactions, and then have it proposed to the multi-sig wallet, making it visible on their UI.

However, when running the script, I get the following error message from the safe-ui service:

ProposeTransactionFailed(422, "{\"nonFieldErrors\":[\"Signer=0x9999999999999999999999999999999999999999 is not an owner or delegate. Current owners=['0x1111111111111111111111111111111111111111', '0x2222222222222222222222222222222222222222', '0x3333333333333333333333333333333333333333']. Delegates=set()\"]}")

The signer address returned is notably not the address present in the ledger wallet at the derivation path used to sign; furthermore, the signer address isn't present on the ledger at all.

At first I assumed that this was because the script executes some transactions before this step with a different address, so I might have been constructing the signature wrong. However, the first address used for broadcasting in the script is also not the address listed as a signer.

I found a similar issue someone had with a javascript framework for interacting with the safe API. The solution they later posted implies that the signature being generated wrong might cause this issue, but it's not immediately obvious to me how I would be doing that wrong here.

[aviggiano]

Hey

Thanks for using this library.

I'm not entirely sure about the error you're experiencing, but the simplest way to use ledger with safe-utils is to pass the derivation path directly to proposeTransactions, without the need to safe.sign. See here for an example.

Does this fix your problem?

[aviggiano]

Take a look at this analysis by copilot

#23

[zacshowa]

Thanks for maintaining the library! It's a fantastic, and ergonomic, improvement for create utility scripts that need to interact with Safe wallets and already existing contracts on chain.

Your suggestion did fix my issue, thanks so much @aviggiano.

I got my original approach, doing this pre-computation of the signature from the README, discussions in issue #15, and PR #17 from some of the code linked by @0xJem.

I applied your suggestion and arrived at:

        //Generate transaction target and data arrays for signing.
        bytes memory transactionDataContract1 = abi.encodeCall(ownable2StepContract1.acceptOwnership, ());
        bytes memory transactionDataContract2 = abi.encodeCall(ownable2StepContract2.acceptOwnership, ());
        bytes memory transactionDataContract3 = abi.encodeCall(ownable2StepContract3.acceptOwnership, ());

        addToBatch(address(contract1), transactionDataContract1);
        addToBatch(address(contract2), transactionDataContract2);
        addToBatch(address(contract3), transactionDataContract3);
        
        // Use the Safe API to propose this batch of transactions on the Web UI, allowing us to collect other signatures.
        return safe.proposeTransactions(batchTargets, batchData, proposerAddress, derivationPath);

I guess the "fix" for this might be to update the README section where it mentions

Proposing a transaction/transactions using a Ledger will also require pre-computing the signature, due to a (current) limitation with forge.

The first step is to pre-compute the signature:

to suggest proposeTransactions as an alternative?

I'm sure there are some more advanced contracts where this pre-computation of the signature is necessary, or useful, but it seems like most users would benefit from jumping to proposeTransactions. I think it doesn't seem to be necessary for me because I'm not using --ledger to run the script.

I'll another test with my original code using delegateCall, like copilot suggests. If it works I think it would be worthwhile to add that detail to the README, and a note in the comments of proposeTransactionsWithSignature like it suggests as well.

[zacshowa]

It turns out Copilot was right. Using Enum.Operation.DelegateCall was the underlying reason for this error!