Salty.IO - jasonxiale's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L42-L55 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59-L69 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L73-L89

Vulnerability details

Impact

after a reject on ManagedWallet.receive, ManagedWallet.proposeWallets and ManagedWallet.changeWallets will be DOSed for ever.

Proof of Concept

In ManagedWallet.proposeWallets, the function will check proposedMainWallet == address(0) to avoid overwriting a previous proposal.

And when confirmationWallet rejects a proposal by calling ManagedWallet.receive, activeTimelock is set to type(uint256).max, but proposedMainWallet isn't updated.

 59     receive() external payable
 60         {
 61         require( msg.sender == confirmationWallet, "Invalid sender" );
 62
 63                 // Confirm if .05 or more ether is sent and otherwise reject.
 64                 // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
 65         if ( msg.value >= .05 ether )
 66                 activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
 67         else
 68                         activeTimelock = type(uint256).max; // effectively never
 69         }

In such case, mainWallet can't call ManagedWallet.proposeWallets any more because of ManagedWallet.sol#L49

At the same time, ManagedWallet.changeWallets also won't work because of activeTimelock is set to type(uint256).max in ManagedWallet.sol#L68, and will always revert in ManagedWallet.sol#L77

Tools Used

VS

Recommended Mitigation Steps

diff --git a/src/ManagedWallet.sol b/src/ManagedWallet.sol
index 7082030..7770d2c 100644
--- a/src/ManagedWallet.sol
+++ b/src/ManagedWallet.sol
@@ -64,8 +64,12 @@ contract ManagedWallet is IManagedWallet
                // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
        if ( msg.value >= .05 ether )
                activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
-       else
+       else {
                        activeTimelock = type(uint256).max; // effectively never
+                       proposedMainWallet = address(0;)
+                       proposedConfirmationWallet = address(0);
+
+             }
         }

Assessed type

DoS

Judge has assessed an item in Issue #878 as 2 risk. The relevant finding follows:

[L-02] typo in Pools.removeLiquidity File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L187 In Pools.sol#L187, the currect check should be:

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol index 7adb563..1fd3f3e 100644 --- a/src/pools/Pools.sol +++ b/src/pools/Pools.sol @@ -184,7 +184,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable

            // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
            // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.

•    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
  

•    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
  
            // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
            if (flipped)
  

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L157-L164 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L235-L273

Vulnerability details

Impact

While a token is added into the whitelist, the DAO will send some SALT as rewards to the paired [weth, token] and [wbtc, token] pool. While the token is removed from the whitelist, the SALT will be stuck.

Proof of Concept

While a token is added into the whilelist, two whitelistPools will be paird, and then bootstrappingRewards * 2 amount of salt will be sent to liquidityRewardsEmitter as pending rewarding for those two poolIds But when a token is removed from the whilelist, the function only removes the paird poolIds from the whilelist, leaving the pending rewards in RewardsEmitter.pendingRewards untouched.

157                 if ( ballot.ballotType == BallotType.UNWHITELIST_TOKEN )
158                         {
159                         // All tokens are paired with both WBTC and WETH so unwhitelist those pools
160                         poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.wbtc() );
161                         poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.weth() );
162
163                         emit UnwhitelistToken(IERC20(ballot.address1));
164                         }

At the same time, RewardsEmitter.performUpkeep can only distribute rewards for the pools which are in the whitelist.

In such case, if there is SALT left for [weth, tokenId], [wbtc, tokenId], those salt will be stucked.

Tools Used

VS

Recommended Mitigation Steps

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L98 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L276 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L320 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L266-L269 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L89 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L122

Vulnerability details

Impact

1. While voting for the ballot in Proposals.castVote, the functions use shares staked in PoolUtils.STAKED_SALT pool as quotas, those shares represent the amount of SALT staked in PoolUtils.STAKED_SALT pool. The issue is that a malicious user can call Proposals.castVote to vote, and then calls Staking.unstake to retrieve SALT back(even there might some SALT lost becaues of expeditedUnstakeFee), and transfers the remaining SALT to another wallet, and restake again, by doing this, the second wallet can also calls Proposals.castVote to vote.

2. By using simliar method, Proposals._possiblyCreateProposal can use this methods to bypass the limitation one user can only have one active proposal

3. And similar to Proposals.castVote, Proposals.requiredQuorumForBallotType can also be manipulated. For example, a whale who owns a large amount of SALT can first Staking.stakeSALT into PoolUtils.STAKED_SALT, and calls Proposals.castVote to vote, then, calls Staking.unstake to withdraw his SALT back. After Staking.unstake, the staking.totalShares( PoolUtils.STAKED_SALT) will be a smaller number, and require less quorum to finalized the ballot.

Proof of Concept

1. staking.userShareForPool is used in Proposals._possiblyCreateProposal and Proposals.castVote, which is a spot value and defined as:

	// Get the user's shares for a specified pool.
	function userShareForPool( address wallet, bytes32 poolID ) public view returns (uint256)
		{
		return _userShareInfo[wallet][poolID].userShare;
		}

2. staking.totalShares is used in Proposals.requiredQuorumForBallotType,which is also a spot value

Tools Used

VS

Recommended Mitigation Steps

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L121-L127 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L81-L118 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L203-L209 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L239-L247 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L248-L255

Vulnerability details

Impact

While Proposals.proposeSetContractAddress and Proposals.proposeWebsiteUpdate are called, the system will first require user to vote for approval to update the price feed and website url. After enough approval is collected, the system will require the user to vote for comfirmation again, this time the system will will create proposal with the name ending with "_confirm".

If a malicious user front-run the confirm proposal with a normal proposal using the confirm proposal's name(_confirm is included), the confirm proposal will revert because the malicious's proposal's name containing "_confirm" already in openBallotsByName[ballotName].

Because of a proposal can't be finalized until ballot.ballotMinimumEndTime, the originl proposal will be blocked until the malicious proposal is finalized.

Proof of Concept

I'll take Proposals.proposeSetContractAddress as an example. while creating a proposal, the proposeSetContractAddress doesn't check if contractName contains "_confirm", and in Proposals._possiblyCreateProposal, the function checks if openBallotsByName[ballotName] and openBallotsByName[ string.concat(ballotName, "_confirm")] are existing in Proposals.sol#L101-L103C12, the function will revert if exists.

 81         function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
 82                 {
 83                 require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );
 84 
     ...
101                 // Make sure that a proposal of the same name is not already open for the ballot
102                 require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
103                 require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );
104 
105                 uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();
106 
107                 // Add the new Ballot to storage
108                 ballotID = nextBallotID++;
109                 ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
110                 openBallotsByName[ballotName] = ballotID;
111                 _allOpenBallots.add( ballotID );
112 
113                 // Remember that the user made a proposal
114                 _userHasActiveProposal[msg.sender] = true;
115                 _usersThatProposedBallots[ballotID] = msg.sender;
116 
117                 emit ProposalCreated(ballotID, ballotType, ballotName);
118                 }

So there will be a case that:

1. Alice calls Proposal.proposeSetContractAddress to update the price feed, and the contractName for the function is update_price_feed1
2. after users vote approval for the proposal, the proposal can be finalized.
3. Alice calls DAO.finalizeBallot to finalize her proposal
4. Bob(the malicious user) doesn't want to change the price feed, so he front-runs Alice's DAO.finalizeBallot tx by calling Proposal.proposeSetContractAddress, in bob's call, he sets contractName as update_price_feed1_confirm
5. when Alice's DAO.finalizeBallot executed, the DAO._executeApproval will create a confirm proposal by calling proposals.createConfirmationProposal
6. the createConfirmationProposal will revert because openBallotsByName[ballotName] is already set by bob's malicious proposal

Tools Used

VS

Recommended Mitigation Steps

while creating a proposal, all the functions( markBallotAsFinalized, proposeParameterBallot, proposeTokenWhitelisting, proposeTokenUnwhitelisting, proposeSendSALT, proposeCallContract, proposeCountryInclusion, proposeCountryExclusion, proposeSetContractAddress, proposeWebsiteUpdate) should check if the name contains "_confirm", if so, revert.

Assessed type

DoS

Judge has assessed an item in Issue #878 as 2 risk. The relevant finding follows:

[L-05] CoreChainlinkFeed.MAX_ANSWER_DELAY is too strict File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L12 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L44-L49 In CoreChainlinkFeed.latestChainlinkPrice, the function checkes if the answerDelay between two rounds is less than CoreChainlinkFeed.MAX_ANSWER_DELAY, if so, 0 is returns as price. But this check is too strict, take BTC/USD as example, for roundId 110680464442257320397, the updatedAt value is 1706586947, and for roundId 110680464442257320398, the updatedAt is 1706590607. So the delay between those two update time is 1706590607 - 1706586947 == 3660. In such cases, CoreChainlinkFeed.latestChainlinkPrice will return 0 if the tx is in front of chainlink’s price update tx. To fix the issue, we can change CoreChainlinkFeed.MAX_ANSWER_DELAY a little larger such as 65 mins

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L221-L236 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L211-L216 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L195-L206

Vulnerability details

Impact

According to Pricing LP tokens | Warp Finance hack and Fair Uniswap's LP Token Pricing,CollateralAndLiquidity.userCollateralValueInUSD and CollateralAndLiquidity.totalCollateralValueInUSD use the wrong algorithm to calcuate user's collateral value. Because of the wrong algorithm, the system might be lost assets.

Proof of Concept

1. InCollateralAndLiquidity.userCollateralValueInUSD, the function first calculates how much WETH/WBTC a user have depending on his share in CollateralAndLiquidity.sol#L229-L233:

221         function userCollateralValueInUSD( address wallet ) public view returns (uint256)
...
229                 // Determine how much collateral share the user currently has
230                 (uint256 reservesWBTC, uint256 reservesWETH) = pools.getPoolReserves(wbtc, weth);
231
232                 uint256 userWBTC = (reservesWBTC * userCollateralAmount ) / totalCollateralShares; <<<--- Here the function calculates how WBTC/WETH a user have depending on his share
233                 uint256 userWETH = (reservesWETH * userCollateralAmount ) / totalCollateralShares;
234
235                 return underlyingTokenValueInUSD( userWBTC, userWETH );
236                 }

And then in CollateralAndLiquidity.underlyingTokenValueInUSD, the function will calcuate market value in USD for a given amount of BTC and ETH

	function underlyingTokenValueInUSD( uint256 amountBTC, uint256 amountETH ) public view returns (uint256)
		{
		// Prices from the price feed have 18 decimals
		uint256 btcPrice = priceAggregator.getPriceBTC();
        uint256 ethPrice = priceAggregator.getPriceETH();

		// Keep the 18 decimals from the price and remove the decimals from the token balance
		uint256 btcValue = ( amountBTC * btcPrice ) / wbtcTenToTheDecimals;
		uint256 ethValue = ( amountETH * ethPrice ) / wethTenToTheDecimals;

		return btcValue + ethValue;
		}

2. In CollateralAndLiquidity.totalCollateralValueInUSD, the function does the same thing as:

You can't just do reserves[0]*prices[0] + reserves[1]*prices[1] when computing TVL in Uniswap LP or you will get rekted. Your code: https://t.co/GEBQ7XKEO7

so similar issue happened to Warp Finance Hack might happen here.

Tools Used

Recommended Mitigation Steps

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L90-L135

Vulnerability details

Impact

In Pools._addLiquidity's implemenation, there are two places that don't conform UniswapV2's whitepaper and algorithm, which might break the system.

Proof of Concept

There are two places in Pools._addLiquidity that don't conform with UniswapV2's implemenation.

1. Initialization of liquidity supply in Pools._addLiquidity, the initialization liquidity supply will be maxAmount0 + maxAmount1

 90         function _addLiquidity( bytes32 poolID, uint256 maxAmount0, uint256 maxAmount1, uint256 totalLiquidity ) internal returns(uint256 addedAmount0, uint256 addedAmount1, uint256 addedLiquidity)
 91                 {
...
 96                 // If either reserve is zero then consider the pool to be empty and that the added liquidity will become the initial token ratio
 97                 if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) )
 98                         {
 99                         // Update the reserves
100                         reserves.reserve0 += uint128(maxAmount0);
101                         reserves.reserve1 += uint128(maxAmount1);
102 
103                         // Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
104                         return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) ); <<<--- Here initialization liquidity supply is calculated
105                         }
106 
...

but in uniswapV2, the liquidity supply is calculated as:

    function mint(address to) external lock returns (uint liquidity) {
        ...
        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
        if (_totalSupply == 0) { <<<--- Here  is used to calculate initialization liquidity supply
            liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
           _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
        } else {
            liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);
        }
        ...
    }

And the quoting from the UniswapV Core paper:

This formula ensures that the value of a liquidity pool share at any time is essentially independent of the ratio at which liquidity was initially deposited

2. Another issue is that while calclate the liquidity, Pools._addLiquidity incorrectly assume a larger amount of token can maintain better numeric resolution in Pools.sol#L128-L135

128                 // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
129                 // Use whichever added amount was larger to maintain better numeric resolution.
130                 // Rounded down in favor of the protocol.
131                 if ( addedAmount0 > addedAmount1)
132                         addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
133                 else
134                         addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;
135                 }

while uniswapV2 using liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1); to calculate the the liquidity in UniswapV2Pair.sol#L123C13-L123C112

To prove the assumption is wrong, suppose there is a pool with tokenX=101e18, and tokenY=10e8 Alice stakes 10e18 TokenX and 99009900 TokenY to make the pool balance, in such case:

1. if we calcuate the lp supply by tokenX, we'll get liquidity = totalLiquidity*10e18/101e18
2. if we calcuate the lp supply by tokenY, we'll get liquidity = totalLiquidity*99009900 / 10e8 In pyhton, we can compare those two values as:

In [7]: (10e18/101e18) > 99009900 / 10e8
Out[7]: True

Which means 99009900 / 10e8 is smaller, so the assumption is not correct

Tools Used

VS

Recommended Mitigation Steps

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol
index 7adb563..a5fe40f 100644
--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
@@ -101,7 +101,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable
                        reserves.reserve1 += uint128(maxAmount1);

                        // Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
-                       return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) );
+                       return ( maxAmount0, maxAmount1, (Math.sqrt(maxAmount0.mul(maxAmount1)));
                        }

                // Add liquidity to the pool proportional to the current existing token reserves in the pool.
@@ -125,6 +125,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable
                reserves.reserve0 += uint128(addedAmount0);
                reserves.reserve1 += uint128(addedAmount1);

+               addedLiquidity = min((totalLiquidity * addedAmount0) / reserve0, (totalLiquidity * addedAmount1) / reserve1);
                // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
                // Use whichever added amount was larger to maintain better numeric resolution.
                // Rounded down in favor of the protocol.

Assessed type

Uniswap

[L-01] DAO.websiteURL doesn't have default value for first 65 days at least.

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L70-L109 In DAO.constructor, DAO.websiteURL doesn't set a default falue. And the only way to set DAO.websiteURL is by calling Proposals.proposeWebsiteUpdate. Since because proposal can't be create in first 45 days because of Proposals.sol#L83, and a ballot can't be finalized within 10 days because of Proposals.sol#L392 is 10 days. And after the ballot is finalized, becaue the proposal is BallotType.SET_WEBSITE_URL, another BallotType.CONFIRM_SET_WEBSITE_URL is creates, which also requires 10 days to finalize. So the protocol won't have websiteURL for 65 days.

[L-02] typo in Pools.removeLiquidity

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L187 In Pools.sol#L187, the currect check should be:

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol
index 7adb563..1fd3f3e 100644
--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
@@ -184,7 +184,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable

                // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
                // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
-        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
+        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

                // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
                if (flipped)

[L-03] ETH will be stuck in ManagedWallet.sol

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59-L69 ManagedWallet.receive uses amount of ETH to confirms or rejects wallet proposals. When ETH is recieved in the contract, there is no way to withdraw the ETH

[L-04] SigningTools._verifySignature might be suffered signature malleability attack

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/SigningTools.sol#L11-L28 SigningTools._verifySignature might be suffered signature malleability attack

[L-05] CoreChainlinkFeed.MAX_ANSWER_DELAY is too strict

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L12 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L44-L49 In CoreChainlinkFeed.latestChainlinkPrice, the function checkes if the answerDelay between two rounds is less than CoreChainlinkFeed.MAX_ANSWER_DELAY, if so, 0 is returns as price. But this check is too strict, take BTC/USD as example, for roundId 110680464442257320397, the updatedAt value is 1706586947, and for roundId 110680464442257320398, the updatedAt is 1706590607. So the delay between those two update time is 1706590607 - 1706586947 == 3660. In such cases, CoreChainlinkFeed.latestChainlinkPrice will return 0 if the tx is in front of chainlink's price update tx. To fix the issue, we can change CoreChainlinkFeed.MAX_ANSWER_DELAY a little larger such as 65 mins

[L-06] PriceAggregator.setInitialFeeds lacks of setting priceFeedModificationCooldownExpiration

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L36-L44 In PriceAggregator.setPriceFeed, priceFeedModificationCooldownExpiration is set at the end of the function, it seems that PriceAggregator.setInitialFeeds should also be set in PriceAggregator.setInitialFeeds

[L-07] Chainlink's latestRoundData return stale or incorrect result

File: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L32-L53

On CoreChainlinkFeed.sol, latestRoundData lacks of check if the returned value indicates stale data Reference: https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/94