[M-02] Failed transactions should not be replayable.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L164

Vulnerability details

Impact

The callSigned function in Caller.sol allows to replay the transaction if the transaction is reverted because the nonce is not updated. This should not be the case, the nonce should be updated even if the transaction is updated.

Assuming a case where userA makes a call using callSigned to the contract AddressDriver.sol’s give function, to send userB someone funds and the function call fails for some reason, eg, if there is not enough funds to send for the give function call.

Now userA adds more than enough funds (double) and makes a call directly using callAs to call give function.

This leaves the first call to callSigned replayable, and userB can call the function again to replay the reverted transaction.

POC

function callSigned(
        address sender,
        address to,
        bytes memory data,
        uint256 deadline,
        bytes32 r,
        bytes32 sv
    ) public payable returns (bytes memory returnData) {
        // slither-disable-next-line timestamp
        require(block.timestamp <= deadline, "Execution deadline expired");
        uint256 currNonce = nonce[sender]++;
        bytes32 executeHash = keccak256(
            abi.encode(
                callSignedTypeHash, sender, to, keccak256(data), msg.value, currNonce, deadline
            )
        );
        address signer = ECDSA.recover(_hashTypedDataV4(executeHash), r, sv);
        require(signer == sender, "Invalid signature");
        return _call(sender, to, data, msg.value);
    }

Nonce not updated above for failed transaction.

Recommendation

Update nonce for every transaction by catching the error of the call.

[GalloDaSballo]

There is some validity to this, but I think QA at best

A reverted TX could be played until the nonce is updated, it cannot be replayed but only run once at any time until the nonce is increased

[CodeSandwich]

This is a duplicate of https://github.com/code-423n4/2023-01-drips-findings/issues/63, so I'll copy-paste my comment.

---

[disagree with severity: QA] That's a correct observation, this is how the contract is designed to work.

I disagree with the proposed solution. Failures burning nonces is a bad design from the signer perspective. If you sign a message, you want it executed, if at some point for whatever reason it can't be, you probably want it to be executed later. If you don't want that, you can set a deadline. Burning nonces also doesn't protect the signer well, if your failing message can be used later with harm to you, the attacker can choose to not execute it, not burn the nonce and wait for the right moment. Burning nonces is also tricky if there's more than 1 signed message floating around, each consecutive one needs to take into account that the previous ones may not have been executed.

I disagree that the described attack is a threat. Even if the signer doesn't set a deadline, they can disable a pending message by signing a dummy transaction with the same nonce and executing it to burn a nonce. This way it's the signer who controls what should happen with the failing message. I disagree that the issue should have high severity, the signer can drop any signed message that hasn't been executed yet.

I think that nonces management can be improved, instead of executing dummy messages Caller could just expose a function for setting the current nonce. This way whole queues of messages can be dropped easily and cheaply. It's a quality of life improvement, so I propose keeping the issue, but lowering severity to low.

[c4-sponsor]

CodeSandwich marked the issue as disagree with severity

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

I believe the finding is best categorized as a risk disclosure, in that end-users can self-rek by making a mistake

However, the logic is consistent with it's own goals

Low Severity per https://github.com/code-423n4/org/issues/53

L

[c4-judge]

GalloDaSballo marked the issue as grade-b