QuickAccManager Smart Contract signature verification can be exploited

[code423n4]

Handle

cmichel

Vulnerability details

Several different signature modes can be used and Identity.execute forwards the signature parameter to the SignatureValidator library. The returned signer is then used for the privileges check:

address signer = SignatureValidator.recoverAddrImpl(hash, signature, true);
// signer will be QuickAccountContract
require(privileges[signer] != bytes32(0), 'INSUFFICIENT_PRIVILEGE');

It's possible to create a smart contract mode signature (SignatureMode.SmartWallet) for arbitrary transactions as the QuickAccManager.isValidSignature uses an attacker-controlled id identity contract for the privileges check. An attacker can just create an attacker contract returning the desired values and the smart-wallet signature appears to be valid:

// @audit id is attacker-controlled
(address payable id, uint timelock, bytes memory sig1, bytes memory sig2) = abi.decode(signature, (address, uint, bytes, bytes));
// @audit this may not be used for authorization, attacker can return desired value
if (Identity(id).privileges(address(this)) == accHash) {
  // bytes4(keccak256("isValidSignature(bytes32,bytes)")
  return 0x1626ba7e;
} else {
  return 0xffffffff;
}

POC

Assume an Identity contract is set up with a QuickAccManager as the privileges account, i.e. privileges[accHash] != 0.

We can construct a SignatureMode.SmartWallet signature for an arbitrary hash:

1. Call Identity.execute(txns, spoofedSignature) where spoofedSignature = abi.encode(attackerContract, timelock=0, sig1=0, sig2=0, address(quickAccountManager), SignatureMode.SmartWallet)
2. This will call recoverAddrImpl(txnsHash, spoofedSignature, true), decode the bytes at the end of spoofedSignature and determine mode = SignatureMode.SmartWallet and wallet = quickAccountManager. It will cut off these arguments and call quickAccountManager.isValidSignature(txnsHash, (attackerContract, 0, 0, 0))
3. The QuickAccManager will decode the signature, construct accHash which is the hash of all zeroes (due to failed signatures returning 0). It will then call attacker.privileges(address(this)) and the attacker contract can return the accHash that matches an account hash of failed signatures, i.e., keccak256(abi.encode(QuickAccount(0,0,0))). The comparison is satisfied and it returns the success value.
4. The checks in Identity.execute pass and the transactions txns are executed.

Impact

Any Identity contract using QuickAccManager can be exploited. Funds can then be stolen from the wallet.

Recommendation

The issue is that QuickAccManager blindly trusts the values in signature. It might be enough to remove the id from the signature and use msg.sender as the identity instead: Identity(msg.sender).privileges(address(this)) == accHash. This seems to work with the current Identity implementation but might not work if this is extended and the isValidSignature is called from another contract and wants to verify a signature on a different identity. In that case, the Identity/SignatureValidator may not blindly forward the attacker-supplied signature and instead needs to re-encode the parameters with trusted values before calling QuickAccManager.

[Ivshti]

great find! Mitigated in https://github.com/AmbireTech/adex-protocol-eth/commit/17c073d037ded76d56d6145faa92c1959fd47226 but still figuring out whether this is the best way to do it

[GalloDaSballo]

May need to sit on this one for another day before I can fully comment

Fundamentally by calling Identity.execute with mostly 0 data, you are able to call back to QuickAccManager. isValidSignature which, due to the implementation of ecrecover at the time, will return valid checks for address(0), allowing to bypass all the logic and returning true for the signature, allowing for the execution of arbitrary code.

Again, need to sit on this one

But wouldn't you also be able to set a malicious smartContractWallet as the IERC1271Wallet, hence you can sidestep the entire logic, as your malicious contract wallet can be programmed to always return true on any input value?

[Ivshti]

@GalloDaSballo this doesn't have to do with address(0)

Using smart wallets for signatures by itself is not a problem - since they authorize as themselves.

The fundamental root of this issue is that ERC 1271 was designed with the assumption that 1 contract = 1 wallet. And as such, isValidSignature only returns true/false. This makes sense, as essentially we're asking the wallet "is this a valid signature from you", and then the wallet decides how to actually validate this it depending on it's own behavior and permissions.

However, the QuickAccManager is a singleton contract - one single QuickAccManager represents multiple users. As such, combining it with ERC 1271 is a logical misunderstanding, as we can't really ask it "is this a valid sig for X identity" through the ERC 1271 interface. So instead, we encode the identity that we're signing as in the sig itself, but then a malicious user could call a top-level identity with a sig that validates in the singleton QuickAccManager, but meant to validate with a differerent identity.

Because what we pass to isValidSignature is opaque data (the smart wallet may be any contract with any logic, not just our QuickAccManager) we can't just peak into the sig and see if it's meant to validate with the caller identity.

Excellent finding IMO

The current mitigation is hacky, and essentially leads to an isValidSignature implementation that is unusable (and doesn't make sense) off-chain, but we prefer it to introducing a new sig type especially for QuickAccManager.

[GalloDaSballo]

@Ivshti To clarify: Would adding privileges[QuickAccountManager] = bytes32(uint(1))enable the exploit?

[Ivshti]

@GalloDaSballo yes, it would. Any authorized quickAcc would enable the exploit

[GalloDaSballo]

I'm starting to get this

The id sent to isValidSignature is an untrusted, unverified address The contract at that address can be programmed to have a function privileges which would return any bytes32 value to match accHash This effectively allows to run arbitrary transactions.

A way to mitigate would be to have a way to ensure the called id is trusted A registry of trusted ids may be effective

The mitigation the sponsor has chosen does solve for only using trusted Identities as in the case of a malicious Identity, the Identity would just validate it's own transaction, not putting other Identities funds at risk.

An alternative solution would be to change theIdentityFactory to use the OpenZeppelin Clones Library (or similar) to ensure that the correct Logic is deployed (by deploying a minimal-proxy pointing to the trusted implementation). This would require a fair tech-lift and would limit the type of deployments that the IdentityFactory can perform.

The exploit was severe and the sponsor has mitigated by checking the msg.sender against the id provided in the signature