sakshamguruji - Random Nonces Can Be Pushed

[sherlock-admin2]

sakshamguruji

medium

Random Nonces Can Be Pushed

Summary

The purpose of a nonce is to uniquely identify a tx/identifier so that the action the nonce is used for can not be replayed , the usual use case for a nonce is that with every nonce used the value is incremented for the next use so that the next time the nonce is assigned it is not the same as before.

Vulnerability Detail

In the function vetoTransaction in SafeGuard.sol

 function vetoTransaction(
        bytes32 transactionHash,
        uint256 nonce
    ) public onlyOwner {
        // Revert if the transaction has already been vetoed
        if (transactionHashes[transactionHash])
            revert PreviouslyVetoed(transactionHash);
        // Mark the transaction as vetoed
        transactionHashes[transactionHash] = true;
        // Add the nonce of the transaction to the nonces array
        nonces.push(nonce);//AUDIT  - any random nonce can be pushed
    }

Even though the function is protected with onlyOwner a previously used nonce can be pushed with the transactionHash. Therefore 2 different transaction hashes might have the same nonce (mistakenly assigned by the trusted party).

Due to this the function checkTransaction (which checks if a tx has been vetoed) ->

for (uint256 i = 0; i < nonces.length; i++) {
            bytes32 transactionHash = IReality(_msgSender()).getTransactionHash(
                to,
                value,
                data,
                operation,
                nonces[i]
            );

This gets the transaction hash (dependent upon the nonce , since the nonce uniquely identifies a hash for let's say a scenario where to , value , data , operation are same for a tx) , therefore if for 2 IDENTICAL tx's with same nonce , this might give erroneous result if one of the tx has been vetoed (then the other also would be marked as vetoed since hash would be same )

Impact

The tx's can not be uniquely identified if mistakenly equal nonce is assigned for 2 identical tx's

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/zodiac/core/SafeGuard.sol#L38

Tool used

Manual Review

Recommendation

Make sure the nonce is incremented each time and then assigned.

[amshirif]

Though there should never be the incorrect submission of a nonce the recommended solution does not address the problem. However an incorrect nonce would prevent the guard from receiving a valid veto.

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/45

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { the function is trusted due to onlyOwner modifier}

[nevillehuang]

request poc

[sherlock-admin2]

PoC requested from @SakshamGuruji3

Requests remaining: 7

[nevillehuang]

@amshirif Theoretically, can 2 transactions share the same nonce? Seems extremely unlikely to me, and given this issue is also dependent on admin making an error not recognizing that.

[fnanni-0]

I think the question should be whether the same transaction can be vetoed at some point but valid later in time. I thought that nonces were being used for that purpose.

For example, a proposal containing a tx to disable a Safe module might be malicious after launch, but might be a valid action after a year. However, if the tx was vetoed, the guard will always block txs given by the same (to, value, data, operation) even if a different nonce is used to veto them, because all nonces are tried in checkTransaction. Is this correct?

If this is not a problem, then I'm not understanding what are nonces being used for.

[amshirif]

So when a snapshot vote with zodiac is submitted a hash is provided associated with the safe and it's nonce. In order for the transaction to be vetoed the nonce must be correctly associated with the transaction or it will not work. So if the wrong nonce was submitted with the transaction hash there would be no way to veto the transaction because in the future that hash has been submitted already.

[fnanni-0]

So when a snapshot vote with zodiac is submitted a hash is provided associated with the safe and it's nonce

Are you referring to this txHash?

So if the wrong nonce was submitted with the transaction hash there would be no way to veto the transaction because in the future that hash has been submitted already.

In the current implementation there is a way to veto it: you could submit a random transaction hash with the correct nonce, right?

Going back to my original question. Let's say proposal A contains [tx1, tx2] and proposal B contains [tx2, tx3]. The owner of SafeGuard vetoes tx2 coming from proposal A. Do you expect tx2 in any other proposal, for example proposal B, to be valid or vetoed?

[nevillehuang]

@fnanni-0 I know what you are saying now I think this while unlikely, could cause a permanent DoS. I will possibly maintain as medium severity. One question though how is the transactionHash built and is the nonce included?

[fnanni-0]

One question though how is the transactionHash built and is the nonce included?

The nonce is included, look here. In the RealityModule nonce means index of the transaction in the array of transactions of a given proposal. In the SafeGuard contract the nonce could be defined like that or in any other way.

[nevillehuang]

@Czar102 @amshirif What are your thoughts for this? It is seemingly alluded to a admin error, but there is a possibility I believe.

[amshirif]

It would be an admin error. The hash is built with the nonce so if the correct hash was submitted but the wrong nonce, then a random hash would need to be submitted with the correct nonce. This is only possible because there are two different disjointed data structures used.

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/45.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.