zzykxx - `checkTransaction()` might run out of gas because of an unbounded loop

[sherlock-admin2]

zzykxx

medium

checkTransaction() might run out of gas because of an unbounded loop

Summary

The function checkTransaction() in SafeGuard.sol might get to a state where it always reverts because it loops over an array that can only increase in size.

Vulnerability Detail

The function checkTransaction() is called by a gnosis safe as a last layer of protection to check whether a transaction should be executed or not. The function is meant to revert if the owner vetoed the transaction via vetoTransaction().

The function vetoTransaction() pushes a new value in the array nonces every time is called and the function checkTransaction() loops over all the elements of nonces every time is called:

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

It's possible for the nonces array to get big enough to cause all calls to revert because it runs out of gas.

Impact

Calls from the gnosis safe to checkTransaction() might always revert, causing a DOS.

Code Snippet

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

Tool used

Manual Review

Recommendation

Retrieve the nonce via msg.sender.nonce() - 1 instead of looping through all the nonces.

Duplicate of #151

[sherlock-admin2]

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

takarez commented:

invalid because { The length of the array is controled by a vetoTransaction() function which has an onlyOwner modifier}