Salty.IO - oakcobalt's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125-L128

Vulnerability details

Impact

Liquidizer will constantly be in shortage of USDS to burn during performUpKeep(), and will always drain POL(protocol owned liquidity) during upkeep.

This is due to the accounting of usdsThatShouldBeBurned being out of sync with the actual token transfer (USDS + WBTC + WETH).

Proof of Concept

Liquidizer.sol is supposed to burn USDS and Salt tokens during each performUpKeep(). The problem is in the USDS flow.

Two sources add to usdsThatShouldBeBurned in Liquidizer.sol: a) During repayUSDS(), the same amount of USDS repaid is increased in usdsThatShouldBeBurned; b) when a user's borrow is undercollateralized and liquidated, their collateral tokens(WBTC, WETH) are transferred to Liquidizer, and the debt amount of USDS is increased in usdsThatShouldBeBurned.

The issue is in flow a):usdsThatShouldBeBurned is increased but no USDS tokens are transferred to Liquidizer.sol. This causes a continuous deficiency in USDS in Liquidzier.

//src/stable/CollateralAndLiquidity.sol
    function repayUSDS(uint256 amountRepaid) external nonReentrant {
...
        // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
        //@audit : this only transfer USDS from caller to CollateralAndLiquidity.sol, but not to liquidizer
|>      usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

        // Have USDS remember that the USDS should be burned
        //@audit :this only increases the state variable burnable amount in Liquidizer.sol, but doesn't actually transfer the received USDS token to Liquidizer.sol
|>      liquidizer.incrementBurnableUSDS(amountRepaid);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125-L128)

As a reference for a correct flow: b) both transfers the token(WBTC+WETH) to Liquidizer and increases usdsThatShouldBeBurned in Liquidizer, such that during liquidizer upkeep, WBTC and WETH will be converted to USDS and then burned. In contrast, a) will not transfer any USDS for liquidizer to burn.

//src/stable/CollateralAndLiquidity.sol
	function liquidateUser( address wallet ) external nonReentrant
...
		// Send the remaining WBTC and WETH to the Liquidizer contract so that the tokens can be converted to USDS and burned (on Liquidizer.performUpkeep)
		wbtc.safeTransfer( address(liquidizer), reclaimedWBTC - rewardedWBTC );
		weth.safeTransfer( address(liquidizer), reclaimedWETH - rewardedWETH );

		// Have the Liquidizer contract remember the amount of USDS that will need to be burned.
		uint256 originallyBorrowedUSDS = usdsBorrowedByUsers[wallet];
		liquidizer.incrementBurnableUSDS(originallyBorrowedUSDS);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L176-L181)

If we compare a) and b), we know in performUpKeep() a) causes usdsThatShouldBeBurned to inflate with no underlying usds transferred, which causes usdsThatShouldBeBurned > usdsBalance in _possiblyBurnUSDS(). And else body will run which withdraw protocol owned liquidity (dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW) and dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);).

//src/stable/Liquidizer.sol
    function _possiblyBurnUSDS() internal {
...
        if (usdsBalance >= usdsThatShouldBeBurned) {
...
        } else {
...
|>          dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
|>          dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L123-L124)

As a result, we see due to not enough usdsBalance to burn, Liquidizer will keep withdrawing POL to compensate.

This violates the intention of (1) having extra usds as a burnerable buffer in liquidizer. (Code comment: // Extra USDS (beyond usdsThatShouldBeBurned) remains in this contract as a burnable buffer in the event of undercollateralized liquidation.); (2) protocol POL will be drained continuously during up keep, when it is unnecessary, causing protocol loss of funds steadily.

Tools Used

Manual Review

Recommended Mitigation Steps

In repayUSDS(), transfer user repaid USDS to Liquidizer.sol.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L68

Vulnerability details

Impact

DOS of proposeWallets() and changeWallets() if the confirmationWallet rejects a proposal. ManagedWallet will be stuck with the same wallet address.

Proof of Concept

Main wallet in ManagedWallet can propose to change main wallet and confirmation wallet address. The setup is the current confirmation wallet address can either confirm or rejects this proposal.

The issue is if the confirmation wallet address rejects the proposal, main wallet can no longer propose a different wallet or change wallets.

(1) Let's see when current confirmation wallet accepts a proposal, the confirmation wallet will send ether (>=0.05 ether) to the contract and this unlock timelock that allows changeWallets() to be called to change wallet address once the timelock passes.

//src/ManagedWallet.sol
    receive() external payable
    	{
...
    	if ( msg.value >= .05 ether )
               //@audit-info note: this updates time locks to allow changeWallets()
|>  		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    	else
               //@audit this disable timelock, but didn't clear value of `proposedMainWallet`
|>			activeTimelock = type(uint256).max; // effectively never
        }

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L68)

When block.timestamp passes activeTimelock, changeWallets() can be called to set the new addresses and clear proposedMainWallet and proposedConfirmationWallet to address(0).

	function changeWallets() external
		{
...
		require( block.timestamp >= activeTimelock, "Timelock not yet completed" );
		// Set the wallets
		mainWallet = proposedMainWallet;
		confirmationWallet = proposedConfirmationWallet;
		// Reset
		activeTimelock = type(uint256).max;
		proposedMainWallet = address(0);
		proposedConfirmationWallet = address(0);

(2) Let's see if the current confirmationWallet rejects the proposal, they send ETH less than 0.05 ether, which disable activeTimelock in receive() (activeTimelock = type(uint256).max;). However, proposedMainWallet is not cleared to address(0). This means a) changeWallets() is disabled due to activeTimelock is disabled and b) proposalWallets() is disabled due to proposedMainWallet is not reset to address(0).

//src/ManagedWallet.sol
	function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external
		{
...
                //@audit : Whenever a confirmation wallet rejects the proposal, this will always revert, due to proposedMainWallet was not reset.
|>		require( proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet." );
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L49C1-L49C97)

Now proposeWallets() and changeWallets() are DOSsed and will always revert. Wallet addresses cannot be proposed and changed again.

As a reference for a correct flow, if the confirmation wallet has accepted the proposal, propose and change wallets would still be allowed for unlimited times.

Tools Used

Manual

Recommended Mitigation Steps

In receive() - else body, reset proposedMainWallet to address(0).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L183 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L142-L143

Vulnerability details

Impact

During price volatitily, aggregated price feed might DOS the liquidation process when the protocol needs liquidiation the most to keep USDS overcollateralized.

Proof of Concept

PriceAggregator.sol compares three price feeds and will pick two feeds that are closest and report the average price of the two.

In _aggregatePrices(), before reporting the average price, a sanity check is performed to compare the difference bewteen the two price (_absoluteDifference(priceA, priceB)). And if the difference is greater than a pre-set tolerance in percentage, it reports 0, which reverts getPriceBTC() and getPriceETH().

This mechanism is fine in normal circumstances, but when the asset price is volatile (e.g. WETH or WBTC price dropping), a higher-than-usual price discrepancy may occur, and the price difference between any two price feeds is likely higher than a preset maximumPriceFeedPercentDifferenceTimes1000.

//src/price_feed/PriceAggregator.sol
	function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
		{
...
		uint256 averagePrice = ( priceA + priceB ) / 2;
		// If price sources are too far apart then return zero to indicate failure
|>		if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
			return 0;
		return averagePrice;
}

	function getPriceBTC() external view returns (uint256 price)
...
		price = _aggregatePrices(price1, price2, price3);
|>		require (price != 0, "Invalid BTC price" );

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L182-L183)

The above suggests whenever WETH or WBTC prices become volatile, the risk increases that getPriceBTC() and getPriceETH() will revert. And this will DOS key protocol liquidation process liquidateUser(). Protocol bad debts cannot be liquidated and USDS can be undercollateralized when the protocol needs liquidation the most.

//src/stable/CollateralAndLiquidity.sol
    function liquidateUser(address wallet) external nonReentrant {
... 
          //@audit canUserBeliquidated(wallet) ->userCollateralValueInUSD()->underlyingTokenValueInUsd(userWBTC,userWETH) -> getPriceBTC() flow reverts during price volatility
|>        require(canUserBeLiquidated(wallet), "User cannot be liquidated");
...
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L145)

liquidation shouldn't be at risk of prolonged reverts during price volatility, this puts USDS pegging and protocol health at risk during critical times.

In addition, PriceAggregator.sol can never be replaced by DAO or protocol during emergencies. This leaves the protocol has no means to handle the above situation.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding a protocol-controlled funciton to replace PriceAggregator with a backup oracle during emergencies.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If zapping fails or loses precision, some arbitrage profits will be stuck in Dao.sol

Proof of Concept

In normal circumstances, when a user adds liquidity (_depositLiquidityAndIncreaseShare()), regardless whether zapping is enabled or not, they are assured to claim any unused token back due to checks on the token amount differences.

//src/staking/Liquidity.sol
    function _depositLiquidityAndIncreaseShare(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        bool useZapping
    )
...
       // If any of the user's tokens were not used, then send them back
        if (addedAmountA < maxAmountA)
|>            tokenA.safeTransfer(msg.sender, maxAmountA - addedAmountA);

        if (addedAmountB < maxAmountB)
|>            tokenB.safeTransfer(msg.sender, maxAmountB - addedAmountB);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L110-L114)

However, such checks on token amount difference are not implemented during protocol upkeeps. In Upkeep.sol, step3() and step4() both add POL to DAO in a manner that amountA and amountB will always equal in ETH value, which in most cases will require zapping to swap and balance ratio of amountA/amountB for the pools. Zapping is always enabled in dao.formPOL().

//src/Upkeep.sol
    function _formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountWETH
    ) internal {
          //@audit note: this split weth in half for amountA and amounB
|>        uint256 wethAmountPerToken = amountWETH >> 1;
        uint256 amountA = pools.depositSwapWithdraw(
            weth,
            tokenA,
            wethAmountPerToken,
            0,
            block.timestamp
        );
        uint256 amountB = pools.depositSwapWithdraw(
            weth,
            tokenB,
            wethAmountPerToken,
            0,
            block.timestamp
        );
        // Transfer the tokens to the DAO
        tokenA.safeTransfer(address(dao), amountA);
        tokenB.safeTransfer(address(dao), amountB);
        // Have the DAO form POL
|>      dao.formPOL(tokenA, tokenB, amountA, amountB);
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L140) But formPOL() doesn't check for actual input amount of tokenA/tokenB, and doesn't know whether zapping succeeds or whether there are still some tokens unused due to precision.

//src/dao/DAO.sol
    function formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountA,
        uint256 amountB
    ) external {
...
        //@audit : although zapping is set as true, but no check on actual input token amount of A and B, if zapping fails or loses precision, tokenA or tokenB will be left in DAO.sol
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            tokenA,
            tokenB,
            amountA,
            amountB,
            0,
            block.timestamp,
|>          true
        );
        emit POLFormed(tokenA, tokenB, amountA, amountB);
    }

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L321) Since DAO.sol is caller for depositLiquidityAndIncreaseShare(), any left over amountA or amountB will be sent to DAO.sol. And DAO.sol doesn't have methods to recover tokens, nor does DAO.sol scrape token balance in other flows. Funds will be stuck in DAO in the form of usds, salt or dai.

Note when zapping fails it will not revert but only passes the original amountA and amountB to pools.addLiquidity().

Upkeep.sol shouldn't assume zapping works 100% or will not leave any tokens behind. And the arbitrage profits stuck can accumulate over time.

Tools Used

Manual

Recommended Mitigation Steps

In formPOL() in DAO.sol, check the return values of collateralAndLiquidity.depositLiquidityAndIncreaseShare() and properly handle the left over tokenA and tokenB. For example, sending left over tokens back to Upkeep.sol and salt can be added back to salt rewards.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L372 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L321 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L132-L133 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L178

Vulnerability details

Impact

Key protocol liquidity management and swapping (formPOL and withdrawPOL) are prone to front-running and the loss can accumulate overtime with upkeep calls.

Note that despite the arbitrage mechanism, front-running is still possible and profitable.

Proof of Concept

In Upkeep.sol, perfromUpkeep() is permissionless and includes multiple protocol liquidity management and token-swapping calls. However, all of the protocol-owned liquidity (POL) related swaps and liquidity management have both slippage and stale tx protections disabled at all times.

step1(), step3(), step4() and step5() all contain unprotected swapping or POL liquidity management calls. In all cases, minimum out value is hardcoded to 0 and tx deadline is block.timestamp. 0 out value allows any amount of swap out or liquidity assets; block.timestamp means the transaction can be settled at any time.

For example, step5() swap remaining WETH (arbitrage profits) into SALT, which will be distributed as rewards across pools. But pools.depositSwapWithdraw() allows any amount of SALT output and allow performUpKeep() to settle at any time. step1() might call Dao.sol to withdrawPOL(). step3()step4() will swap tokens to deposit liquidity through formPOL().

See locations of above-mentioned cases:

//src/Upkeep.sol
    function step5() public onlySameContract {
...
        uint256 amountSALT = pools.depositSwapWithdraw(
            weth,
            salt,
            wethBalance,
            0,
            block.timestamp
        );
...

    function _formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountWETH
    ) internal {
        uint256 amountA = pools.depositSwapWithdraw(
            weth,
            tokenA,
            wethAmountPerToken,
            0,
            block.timestamp
        );
        uint256 amountB = pools.depositSwapWithdraw(
            weth,
            tokenB,
            wethAmountPerToken,
            0,
            block.timestamp
        );
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L132-L133)

//src/dao/DAO.sol
	function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
...
		collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );

	function withdrawPOL( IERC20 tokenA, IERC20 tokenB, uint256 percentToLiquidate ) external
...
		(uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block.timestamp );

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L372C1-L372C156)

Even though there is an arbitrage mechanism, front-running is still possible and profitable. And when both slippage and stale tx protections are disabled, front-running is easier and the loss will accumulate over time.

Tools Used

Manual

Recommended Mitigation Steps

Although difficult to implement in the permissionless context, consider implementing priceFeed to ensure a reasonable minimal output amount for WETH to usds, dai and salt swap.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L65-L66

Vulnerability details

Impact

Confirmation wallet can bypass 30-day timelock completely or postpone approved wallet change infinitely

Proof of Concept

In ManagedWallet.sol, confirmation wallet can confirm a proposal to change wallet addresses by sending greater than 0.05 ether to the contract.

However, the current implementation is flawed and allows two unsafe behaviors against intended design: (1) 30-day timelock bypass: There should be 30-day timelock after a proposal. But this can be bypassed by the current confirmation wallet send greater than 0.05 ether to the contract at the very beginning ( e.g. after contract deployment)

This way activeTimelock will be set to a known timestamp well before a proposal is made. Then when the mainwallet makes a proposal through proposeWallets(). The new proposedMainWallet can call changeWallets() instantly due to block.timestamp already passed a historic activeTimelock.

    receive() external payable
    	{
    	require( msg.sender == confirmationWallet, "Invalid sender" );

		// Confirm if .05 or more ether is sent and otherwise reject.
		// 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

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L65-L66)

	function changeWallets() external
		{
		// proposedMainWallet calls the function - to make sure it is a valid address.
		require( msg.sender == proposedMainWallet, "Invalid sender" );
                //@audit : this require will pass without 30day timelock due to activeTimelock is a historic timestamp
|>		require( block.timestamp >= activeTimelock, "Timelock not yet completed" );

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L77) (2) infinite postpone of a confirmed proposal: Confirmation wallet can also postpone a confirmed proposal infinitely by sending 0.05 ether to the contract before the first 30-day timelock is passed, which effectively reset activeTimelock to a new future timestamp. 30-day timelock can be extended to whenever.

Tools Used

Manual

Recommended Mitigation Steps

In receive()- if ( msg.value >= .05 ether ) body, add checks to ensure proposedMainWallet != address(0) && activeTimelock == type(uint256).max

Assessed type

Invalid Validation

Low-01: Users can vote forever till a ballot is finalized. Voting has no deadline before a permissionless finalization call results in vote racing and unfair outcomes.

In Proposals.sol, users can vote techinically forever until a permissionless finalization call. This allows users to race vote and call for finalization against each other for every proposal. Two users might try to front run each other's call to finalizeBallot().

Although technically ok, this might result in a temporary state of vote change to decide the outcome. A person who can submit more gas or is more experienced in front-running might have a better chance of winning a proposal, as supposed to allow all users of various technical proficiency levels to make their votes counted fairly.

//src/dao/Proposals.sol
	function castVote( uint256 ballotID, Vote vote ) external nonReentrant
		{
...
		//@audit : this only checks whether ballot has been finalized/executed. But it allows vote to be cast any time before a call to `finalizeBallot()`
|>		require( ballot.ballotIsLive, "The specified ballot is not open for voting" );
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L264)

For reference, ballotIsLive will only be turned off at the end of finalizeBallot() in markBallotAsFinalized(). So voting can go on till the final proposal execution.

	function markBallotAsFinalized( uint256 ballotID ) external nonReentrant
...
		_allOpenBallots.remove( ballotID );

		ballot.ballotIsLive = false;
...

Recommendations: Consider adding a Voting duration parameter to constraint each proposal to a fixed voting deadline before finalizeBallot() is allowed.

Low-02: Incorrect comment - Contract doesn't allow aidrop participants to vote on country exclusions, in conflict with comments.

In BootstrapBallot.sol, contract comment says // Allows airdrop participants to vote on whether or not to start up the exchange and which countries should be initially excluded from access.

Current implementation only allows airdrop participants to vote on starting exchange, but no means to vote on countries.

Comments are incorrect, if the intention is allowing them to vote on country exclusions, this is not implemented.

Recommendations: Correct comment.

Low-03: Incorrect assumption of chainlink price feed decimals

In CoreChainlinkFeed.sol, all chainlink price feed is assumed to be in 8 decimals. // Convert the 8 decimals from the Chainlink price to 18 decimals This is only true for non-ETH pairs and is fine for fetching collateral prices (WETH, WBTC). However, this will be problematic if lastChainlinkPrice() is used to price ETH pairs which will be in 18 decimals.

//src/price_feed/CoreChainlinkFeed.sol

function latestChainlinkPrice(
        AggregatorV3Interface chainlinkFeed
    ) public view returns (uint256) {
        int256 price = 0;
...
        // Convert the 8 decimals from the Chainlink price to 18 decimals
        //@audit price will be incorrectly scaled for ETH pairs (in 18 decimals)
 |>     return uint256(price) * 10 ** 10;

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L58) Recommendations: Currently latestChainlinkPrice() allows take in any chainlinkFeed which might not be Non-eth pair. Consider adding a check to see whether the chainlinkFeed address input is only for BTC/USD or ETH/USD.

Low-04: Dust and extremely uneven reserve ratio can be established by the first liquidity depositor

When a token is whitelisted by DAO, (WETH, token) and (WBTC, token) will be deployed with zero reserve assets inside. And current impelmentation allows dust liquidity deposit and any reserve ratio for the first depositor.

//src/pools/Pools.sol
    function _addLiquidity(
        bytes32 poolID,
        uint256 maxAmount0,
        uint256 maxAmount1,
        uint256 totalLiquidity
    )
...
        if ((reserve0 == 0) || (reserve1 == 0)) {
            // Update the reserves
|>            reserves.reserve0 += uint128(maxAmount0);
|>            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));
        }
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L97-L104)

There are no checks to see of maxAmount0 and maxAmount1 are dust amount, and no check on the ratio of the two when it is the very first deposit.

When the first depositor adds extremely uneven reserves at a dust cost, this allows pool reserve ratios to be manipulated from the beginning.

Recommendations: Consider not allowing a dust amount of liquidity adding, and also consider adding a recommended reserve ratio value from the ballot to use as a reference for the initial deposit.

Low-05: _userUnstakeIDS is not updated when unstaking is canceled or claimed. This causes unstakesForUser to still retrieve canceled or claimed unstakes as pending

In Staking.sol, a user can unstaked their SALT tokens which starts a delayed process for SALT token claiming. A user can also cancel their unstake request through cancelUnstake() which allows them to continuously gain staking rewards. Otherwise, user can claim their staked token when time has passed through recoverSALT().

The issue is cancelUnstake() and recoverSALT() don't update users staking status completely. User's unstakeID will never be deleted from _userUnstakeIDs[msg.sender] array. This will results in user shows up in unstakesForUser() view function query as having pending unstakes even though user already canceled or claimed their unstakes. (1)

//src/staking/Staking.sol
    function cancelUnstake(uint256 unstakeID) external nonReentrant {
...
        //@audit note: this doesn't remove `unstakeID` from _userUnstakeIDs[msg.sender] arrays.
        u.status = UnstakeState.CANCELLED;
        emit UnstakeCancelled(msg.sender, unstakeID);
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L95) (2)

//src/staking/Staking.sol
function recoverSALT( uint256 unstakeID ) external nonReentrant
...
		u.status = UnstakeState.CLAIMED;

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L108C1-L108C35)

Recommendations: Consider removing unstakeID from _userUnstakeIDs[msg.sender] array. Or in unstakesForUser() function, filter out unstakeID status that is not pending.

Low-06: consider using Openzeppelin's ECDSA library to prevent risk of signature malleability inherent in ecrecover method.

In SingingTools.sol, pre-compiled ecrecover method is used to verify signer address. ecrecover method is vulnerable to signature malleability issues due to the nature of ECDSA algorithm. Consider prevent the risk by using Openzeppelin's ECDSA library.

    function _verifySignature(
        bytes32 messageHash,
        bytes memory signature
    ) internal pure returns (bool) {
...
        address recoveredAddress = ecrecover(messageHash, v, r, s);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/SigningTools.sol#L26)

Recommendations: Use Openzeppline's ECDSA library.

Low-07: Consider adhering to EIP-712 signed structure data method to prevent potential signature relay attacks in occasions such as AccessManager upgrade

In AccessManager.sol, a user needs to submit protocol signed signature as a proof of access to be granted access to deposit liquidity.

The issue is the messageHash that signature is signed on doesn't adhere to EIP-712 signed structure data standard and might be vulnerable for signature relay attacks in the future.

EIP-712 suggests multiple fields to be included in signed message as domain separator including version, name,etc., which prevents message collision from different versions of contracts or dapps.

Current messageHash only includes block.chainid, geoVersion and wallet. If AccessManger.sol is upgraded to a new contract, previous geoVersion number and same user wallet can be the same, which might allow users to use old signatures to get access even though their country is excluded.

//src/AccessManager.sol
    function _verifyAccess(
        address wallet,
        bytes memory signature
    ) internal view returns (bool) {
        bytes32 messageHash = keccak256(
            abi.encodePacked(block.chainid, geoVersion, wallet)
        );
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/AccessManager.sol#L53)

Recommendations: Follow EIP-712 to include field such as contract version to prevent signature relays.

Low-08: When reserve token is less than 18 decimals, r0,r1,z0,z1 might be overscaled during zapping.

In PoolMath.sol, _zapSwapAmount() will normalize reserve0, reserve1, zapAmount0 and zapAmount1 to the same precision if maximumMSB>80. But _maximumMSB() only return the largest msb of the four vales, which means the other three values can be much smaller than 80.

As an example, if msb of r0 is 90, msb of z1 is 10. if (maximumMSB > 80) will be true and shift = maximumMSB - 80 which is 10. In this case z1 will be shifted right by 10, which results in 0 value for z1. z1 will be oversacled and zapping will fail.

Such example could be more common when z1 is a token with much smaller decimal than r0.

//src/pools/PoolMath.sol
    function _zapSwapAmount(
        uint256 reserve0,
        uint256 reserve1,
        uint256 zapAmount0,
        uint256 zapAmount1
    ) internal pure returns (uint256 swapAmount) {
...
        if (maximumMSB > 80) shift = maximumMSB - 80;
...
        uint256 r0 = reserve0 >> shift;
        uint256 r1 = reserve1 >> shift;
        uint256 z0 = zapAmount0 >> shift;
        uint256 z1 = zapAmount1 >> shift;
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L161-L168)

Recommendations: Consider normalizing the input token decimals to a consistent decimal before performing zapping.