`QuickAccManager.sol#send()` Potential replay attack

[code423n4]

Handle

WatchPug

Vulnerability details

In QuickAccManager.sol#send(), address(identity) is not included in the txHash, makes it possible to replay the transaction when the same QuickAccount (accHash) controls multiple Identity.

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/QuickAccManager.sol#L55-L79

function send(Identity identity, QuickAccount calldata acc, DualSig calldata sigs, Identity.Transaction[] calldata txns) external {
    bytes32 accHash = keccak256(abi.encode(acc));
    require(identity.privileges(address(this)) == accHash, 'WRONG_ACC_OR_NO_PRIV');
    uint initialNonce = nonces[address(identity)];
    // Security: we must also hash in the hash of the QuickAccount, otherwise the sig of one key can be reused across multiple accs
    bytes32 hash = keccak256(abi.encode(
        address(this),
        block.chainid,
        accHash,
        nonces[address(identity)]++,
        txns,
        sigs.isBothSigned
    ));
    if (sigs.isBothSigned) {
        require(acc.one == SignatureValidator.recoverAddr(hash, sigs.one), 'SIG_ONE');
        require(acc.two == SignatureValidator.recoverAddr(hash, sigs.two), 'SIG_TWO');
        identity.executeBySender(txns);
    } else {
        address signer = SignatureValidator.recoverAddr(hash, sigs.one);
        require(acc.one == signer || acc.two == signer, 'SIG');
        // no need to check whether `scheduled[hash]` is already set here cause of the incrementing nonce
        scheduled[hash] = block.timestamp + acc.timelock;
        emit LogScheduled(hash, accHash, signer, initialNonce, block.timestamp, txns);
    }
}

PoC

1. Alice set up 2 Identity proxy contracts and top up with 1000 USDC each;
2. Alice send 1000 USDC to Bob with QuickAccManager.sol#send() from Identity 1;
3. Bob or the attacker can replay the transaction above with the same parameters except the first parameter changed to Identity 2, and Bob will receive another 1000 USDC from Identity 2.

Recommendation

Consider including address(identity) in txHash.

[Ivshti]

resolved here: https://github.com/code-423n4/2021-10-ambire-findings/issues/23

[GalloDaSballo]

Duplicate of #39