`QuickAccManager.sol` Potential replay attack

[code423n4]

Handle

WatchPug

Vulnerability details

[This issue is possibly duplicate with a previous issue named "Potential replay attack"]

In QuickAccManager.sol#sendTxns() and QuickAccManager.sol#sendTransfer(), address(identity) is not included in the txHash, makes it possible to replay the transaction on another Identity when the same pair of keys controls multiple Identity.

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

function sendTransfer(Identity identity, QuickAccount calldata acc, bytes calldata sigOne, bytes calldata sigTwo, Transfer calldata t) external {
    require(identity.privileges(address(this)) == keccak256(abi.encode(acc)), 'WRONG_ACC_OR_NO_PRIV');

    bytes32 hash = keccak256(abi.encodePacked(
        '\x19\x01',
        DOMAIN_SEPARATOR,
        keccak256(abi.encode(TRANSFER_TYPEHASH, t.token, t.to, t.amount, t.fee, nonces[address(identity)]++))
    ));
    require(acc.one == SignatureValidator.recoverAddr(hash, sigOne), 'SIG_ONE');
    require(acc.two == SignatureValidator.recoverAddr(hash, sigTwo), 'SIG_TWO');
    Identity.Transaction[] memory txns = new Identity.Transaction[](2);
    txns[0].to = t.token;
    txns[0].data = abi.encodeWithSelector(IERC20.transfer.selector, t.to, t.amount);
    txns[1].to = t.token;
    txns[1].data = abi.encodeWithSelector(IERC20.transfer.selector, msg.sender, t.fee);
    identity.executeBySender(txns);
}

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

function sendTxns(Identity identity, QuickAccount calldata acc, bytes calldata sigOne, bytes calldata sigTwo, Txn[] calldata txns) external {
    require(identity.privileges(address(this)) == keccak256(abi.encode(acc)), 'WRONG_ACC_OR_NO_PRIV');

    // hashing + prepping args
    bytes32[] memory txnBytes = new bytes32[](txns.length);
    Identity.Transaction[] memory identityTxns = new Identity.Transaction[](txns.length);
    for (uint256 i = 0; i < txns.length; i++) {
        txnBytes[i] = keccak256(abi.encode(TXNS_TYPEHASH, txns[i].description, txns[i].to, txns[i].value, txns[i].data));
        identityTxns[i].to = txns[i].to;
        identityTxns[i].value = txns[i].value;
        identityTxns[i].data = txns[i].data;
    }
    bytes32 txnsHash = keccak256(abi.encodePacked(txnBytes));
    bytes32 hash = keccak256(abi.encodePacked(
        '\x19\x01',
        DOMAIN_SEPARATOR,
        keccak256(abi.encode(BUNDLE_TYPEHASH, nonces[address(identity)]++, txnsHash))
    ));
    require(acc.one == SignatureValidator.recoverAddr(hash, sigOne), 'SIG_ONE');
    require(acc.two == SignatureValidator.recoverAddr(hash, sigTwo), 'SIG_TWO');
    identity.executeBySender(identityTxns);
}

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#sendTransfer() or QuickAccManager.sol#sendTxns() 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/AmbireTech/adex-protocol-eth/commit/f70ca38f368da30c9881d1ee5554fd0161c94486

[Ivshti]

duplicate of #23

[GalloDaSballo]

Finding is valid, and documented in #23

Setting this as invalid as this is a duplicate from the same warden