Overwriting Timestamp Clash would allow bad Actors to Insert Sensitive Voting PowerSnapshot Since it would not end up in Storage

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L1032

Vulnerability details

Impact

Bad Actors can insert multiple Sensitive Voting PowerSnapshot at a single timestamp and interact with the contract without the data ending up in storage, all this can happen in a single function call. Bad Actor can use this to acquire extra voting power by delegating.

Proof of Concept

 // Append a new voting power snapshot, overwriting the last one if possible.
    function _insertVotingPowerSnapshot(address voter, VotingPowerSnapshot memory snap) private {
        emit PartyVotingSnapshotCreated(
            voter,
            snap.timestamp,
            snap.delegatedVotingPower,
            snap.intrinsicVotingPower,
            snap.isDelegated
        );

        VotingPowerSnapshot[] storage voterSnaps = _votingPowerSnapshotsByVoter[voter];
        uint256 n = voterSnaps.length;
>>>     // If same timestamp as last entry, overwrite the last snapshot, otherwise append.
        if (n != 0) {
            VotingPowerSnapshot memory lastSnap = voterSnaps[n - 1];
>>>         if (lastSnap.timestamp == snap.timestamp) {
                voterSnaps[n - 1] = snap;
                return;
            }
        }
        voterSnaps.push(snap);
    }

As seen in the comment description and implementation above if the timestamp is the same it should be overwritten. So in simple term if series of voting Power snapshot is to be inserted into storage for a voter at a given timestamp only the last will be saved in storage. The problem with this is a bad actor can take advantage of this and use the voting power before it would be overwritten, a sample attack would look like this
Using a single function call and timestamp

1. Actor _inserts VotingPowerSnapshot to acquire intrinsic Voting Power.
2. Actor uses this power to vote by calling the accept(...) function.
3. Actor Delegates voting Power to a delegate address by calling delegateVotingPower(...).
4. The Delegates uses this same voting power on behalf of the Actor to vote again in the same timestamp since the actor cannot vote again due to validation at L627, the Delegate votes by also by calling the accept(...) function without any problem which allows Actor to indirectly exercise excess voting right.

Note: Attack does not necessarily have to follow this pattern, but this vulnerability can be taken advantage of.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of overwriting snapshot at same timestamp before storage, the protocol would be more secure not allowing snapshot at the same timestamp at all in the first place. This can be done by validating that previous timestamp is not equal to block.timestamp at the time of inserting snapshot i.e in the _adjustVotingPower(...) & _rebalanceDelegates(...) functions

   // Increase `voter`'s intrinsic voting power and update their delegate if delegate is nonzero.
    function _adjustVotingPower(address voter, int192 votingPower, address delegate) internal {
        ...
        VotingPowerSnapshot memory newSnap = VotingPowerSnapshot({
            timestamp: uint40(block.timestamp),
            delegatedVotingPower: oldSnap.delegatedVotingPower,
            intrinsicVotingPower: (oldSnap.intrinsicVotingPower.safeCastUint96ToInt192() +
                votingPower).safeCastInt192ToUint96(),
            isDelegated: delegate != voter
        });
+++     require( _getLastVotingPowerSnapshotForVoter(voter).timestamp != block.timestamp ); //avoid time clash before inserting new one
        _insertVotingPowerSnapshot(voter, newSnap);
        delegationsByVoter[voter] = delegate;

       ...
    }

    // Update the delegated voting power of the old and new delegates delegated to
    // by `voter` based on the snapshot change.
    function _rebalanceDelegates(
       ...
    ) private {
        if (newDelegate == address(0) || oldDelegate == address(0)) {
            revert InvalidDelegateError();
        }
        if (oldDelegate != voter && oldDelegate != newDelegate) {
            // Remove past voting power from old delegate.
            VotingPowerSnapshot memory oldDelegateSnap = _getLastVotingPowerSnapshotForVoter(
                oldDelegate
            );
            VotingPowerSnapshot memory updatedOldDelegateSnap = VotingPowerSnapshot({
                timestamp: uint40(block.timestamp),
                delegatedVotingPower: oldDelegateSnap.delegatedVotingPower -
                    oldSnap.intrinsicVotingPower,
                intrinsicVotingPower: oldDelegateSnap.intrinsicVotingPower,
                isDelegated: oldDelegateSnap.isDelegated
            
+++     require( _getLastVotingPowerSnapshotForVoter(oldDelegate).timestamp != block.timestamp ); //avoid time clash before inserting new one
            _insertVotingPowerSnapshot(oldDelegate, updatedOldDelegateSnap);
        }
        if (newDelegate != voter) {
            // Not delegating to self.
            // Add new voting power to new delegate.
            VotingPowerSnapshot memory newDelegateSnap = _getLastVotingPowerSnapshotForVoter(
                newDelegate
            );
            uint96 newDelegateDelegatedVotingPower = newDelegateSnap.delegatedVotingPower +
                newSnap.intrinsicVotingPower;
            if (newDelegate == oldDelegate) {
                // If the old and new delegate are the same, subtract the old
                // intrinsic voting power of the voter, or else we will double
                // count a portion of it.
                newDelegateDelegatedVotingPower -= oldSnap.intrinsicVotingPower;
            }
            VotingPowerSnapshot memory updatedNewDelegateSnap = VotingPowerSnapshot({
                timestamp: uint40(block.timestamp),
                delegatedVotingPower: newDelegateDelegatedVotingPower,
                intrinsicVotingPower: newDelegateSnap.intrinsicVotingPower,
                isDelegated: newDelegateSnap.isDelegated
            });
+++     require( _getLastVotingPowerSnapshotForVoter(newDelegate).timestamp != block.timestamp ); //avoid time clash before inserting new one
            _insertVotingPowerSnapshot(newDelegate, updatedNewDelegateSnap);
        }
    }

Assessed type

Timing

[ydspa]

File: contracts\party\PartyGovernance.sol
634:         uint96 votingPower = getVotingPowerAt(msg.sender, values.proposedTime - 1, snapIndex);

Invalid

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[Topmark1]

File: contracts\party\PartyGovernance.sol
634:         uint96 votingPower = getVotingPowerAt(msg.sender, values.proposedTime - 1, snapIndex);

Invalid

I believe this report is valid and this comment is not any way related to the issue. This report is about the wrong use of overwrite for snapshot in event of timestamp clash. A Reversion is correct instead of overwriting snapshot

[gzeon-c4]

The comment is relevant because it shows only the previous block snapshot will be used, which is immune from same block manipulation.