Unbounded nonces array

Summary

Nonces array can grow too large to fit the iteration in one block.

Vulnerability Detail

SafeGuard lets the owner to veto tx nonce:

    uint256[] public nonces;

    function vetoTransaction(
        bytes32 transactionHash,
        uint256 nonce
    ) public onlyOwner {
         ...
        nonces.push(nonce);
    }

nonces is a dynamic array, and it only grows with new values over time. Later, when validating the tx, it iterates over all the nonces:

for (uint256 i = 0; i < nonces.length; i++)

This loop can eventually grow so large as to make future operations of the contract cost too much gas to fit in a block.

Impact

Iterating through the arrays of unknown sizes might consume all the gas provided (run out of gas) if too many elements are pushed. This might be less relevant on other chains where gas limits are lifted, but concerning if deployed on Mainnet. The sponsor confirmed that SafeGuard will be deployed on Mainnet: "Some of the contracts will also be deployed to Ethereum." "Currently the only contracts would be the Telcoin distributor or the safe guard. Everything else would be on polygon".

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

Consider refactoring the codebase to avoid such iterations. Add upper limits, the possibility to remove nonces, and other appropriate improvements.

Duplicate of #151

sherlock-audit / 2024-01-telcoin-judging

HonorLt - Unbounded nonces array #185