SafeGuard::checkTransaction() function is not designed with gas efficiency, the implementation can result in DOS if nonces grow very large.

Summary

checkTransaction() function could result in DOS if the nonces array grow very large.

Vulnerability Detail

Impact

checkTransaction() function loops through nonce array and tries to check if the transaction has been vetoed, but as time passes by, the number of nonces could be very large and this could result in gas consumption result in DOS.

The code design is not efficient for blockchain.

Code Snippet

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

 for (uint256 i = 0; i < nonces.length; i++) {
            bytes32 transactionHash = IReality(_msgSender()).getTransactionHash(
                to,
                value,
                data,
                operation,
                nonces[i]
            );
            require(
                !transactionHashes[transactionHash],
                "SafeGuard: transaction has been vetoed"
            );
   }

Tool used

Manual Review

Recommendation

Maintain the nonce to a hash based on other elements of the transaction, excluding nonce. This way, the mapped nonce can be retreived and used to generate the transaction hash.

Generate hash for (to,value,data,operation) and use that to find nonce.

Duplicate of #151

sherlock-audit / 2024-01-telcoin-judging

ravikiran.web3 - SafeGuard::checkTransaction() function is not designed with gas efficiency, the implementation can result in DOS if nonces grow very large. #152