Salty.IO - peanuts's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L70-L86

Vulnerability details

Impact

Users cannot earn rewards from the pool. Also, users cannot withdraw their liquidity once they deposit since _decreaseUserShare will always fail as claimableRewards will be too big.

Explanation

When a user deposits liquidity into a pool, _increaseUserShare() will be called and the totalRewards, totalShares, virtualRewards and userShares will be updated.

                uint256 existingTotalShares = totalShares[poolID];


                // Determine the amount of virtualRewards to add based on the current ratio of rewards/shares.
                // The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool.
                // The virtual rewards will be deducted later when calculating the user's owed rewards.
>       if ( existingTotalShares != 0 ) // prevent / 0
                {
                        // Round up in favor of the protocol.
                        uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );


>                       user.virtualRewards += uint128(virtualRewardsToAdd);
>               totalRewards[poolID] += uint128(virtualRewardsToAdd);
                }


                // Update the deposit balances
>               user.userShare += uint128(increaseShareAmount);
>               totalShares[poolID] = existingTotalShares + increaseShareAmount;

Take note that only if existingTotalShares is not zero, then the virtual rewards will be calculated.

When a pool is first whitelisted and created, there is no default liquidity deposited into the pool.

The first depositor can deposit 1 wei of share (or rather, a small amount of share, will be explained later) so that the existingTotalShare becomes a small number. Then, he can deposit a huge liquidity so that the totalRewards of the pool will become a huge amount. Subsequently, anyone who deposits in the same pool will also have more rewards than intended.

This assumes that the pool has some salt reward added to it in the first place, or if the depositor adds the reward himself.

1 wei of added share is not possible because of the check of dust amount when adding liquidity. The minimum added share would then be 200 (100 wei each)

        function addLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 totalLiquidity ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
                {
                require( msg.sender == address(collateralAndLiquidity), "Pools.addLiquidity is only callable from the CollateralAndLiquidity contract" );
                require( exchangeIsLive, "The exchange is not yet live" );
                require( address(tokenA) != address(tokenB), "Cannot add liquidity for duplicate tokens" );


>               require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
>               require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

Proof of concept

1. The pool is whitelisted, no one added any liquidity yet
2. The first depositor or someone else adds rewards to the pool, say 10000e18 SALT.
3. The first depositor adds in 200 wei of shares to the pool. Since the existingTotalShares is 0, the totalRewards part is skipped.
4. The first depositor then adds 10e18 worth of shares to the pool (say the pool tokens is in 18 decimals). The totalRewards becomes 10000e18 * 10e18 / 100 which is 1000e36. The totalRewards become 1000e36.
5. ExistingTotalShares is updated to 10e18 + 200. The totalRewards variable is now manipulated already

The pool is now unable to earn rewards because totalRewards is too big.

Also, users will find themselves adding liquidity into the pool but unable to withdraw any liquidity since the value of totalRewards is too big. As a result, the value of claimableRewards will be too big as well, which will lead to a revert when transferring salt tokens.

                // In the event that virtualRewards are greater than actual rewards - claimableRewards will stay zero.
                if ( virtualRewardsToRemove < rewardsForAmount )
                        claimableRewards = rewardsForAmount - virtualRewardsToRemove;


                // Send the claimable rewards
>               if ( claimableRewards != 0 )
>                       salt.safeTransfer( wallet, claimableRewards );

Tools Used

VSCode

Recommended Mitigation Steps

Recommend the protocol start out with an initial liquidity for all whitelisted pool to prevent users from manipulating the totalRewards variable. 1e18 worth of pool tokens will be enough to start, and the DAO can pool the liquidity from the Protocol-owned liquidity.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L259-L283 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L97-L118

Vulnerability details

Proof of Concept

To cast a vote (Proposals.sol), the voter has to have some SALT staked in the pool. This staked SALT, or xSALT, refers to their voting power.

                uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
                require( userVotingPower > 0, "Staked SALT required to vote" );

Users can cast their votes and then unstake their SALT. The process of unstaking SALT will not affect their current voting power, which should not be the case.

This is a potential problem for the protocol. Whales that decides to not participate in the protocol anymore can vote negatively for all the current proposals and then unstake their position.

They will not be affected in any way but it will grief all the incoming proposals.

The whales action will also affect the quorum percentage since staking.totalShares is part of the calculation for the required quorum. This means that the quorum assumes that the whale's shares are not there already but they still have their votes counted.

        function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
                {
                // The quorum will be specified as a percentage of the total amount of SALT staked
                uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
                require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

To give a concrete example: Say there are 5 million total SALT staked. Alice the whale has 1 million of that total SALT staked.

The current ballot percentage is 10%, so to reach quorum, the total number of votes must be 500k.

Alice wants to leave the protocol, so she decides to grief all the proposals by voting for the negative. Afterwards, she unstakes her 1 million SALT.

The total number of SALT staked is now 4 million. The current quorum votes needed is now 400k. Alice still has 1 million votes for the negative, which is unfair because the quorum is lowered due to her unstaking but she still has a say in voting.

Impact

The balloting procedure can be potentially griefed by unstakers.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend deleting all votes when a user unstakes their SALT.

The unstake function has to check all the ballotID that the user participated in and make sure he does not have votes in any of them.

Assessed type

Context

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

[L-08] CoreChainlinkFeed uses the BTC_USD address when it should be using the WBTC-BTC one as well CoreChainlinkFeed assumes that the price of BTC is equal to the price of WBTC.

This is the BTC/USD feed:

https://data.chain.link/feeds/ethereum/mainnet/btc-usd

This is the WBTC/BTC feed:

https://data.chain.link/feeds/ethereum/mainnet/wbtc-btc

Note that the WBTC/BTC is not 1:1. Users may not get the actual WBTC price (since protocol is using WBTC instead of BTC) which will affect collaterization ratio and future liquidations.

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreChainlinkFeed.sol#L32-L39

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

[L-02] In ManagedWallet.sol, the activeTimelock can be set even before proposeWallets is called, so the activeTimelock can be bypassed. The confirmation wallet can send 0.05 ether even before proposeWallets() function is called to skip the TIMELOCK_DURATION.

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 )
            //@audit - Skip this part by depositing 0.05 ether before calling `proposeWallets()`
            activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    else
                    activeTimelock = type(uint256).max; // effectively never
    }

Make sure the activeTimelock is only set once the proposeWallet() function is called.

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

[L-01] Last few dust amounts in the emission contract will be truncated to zero

This is the calculation for emission:

uint256 saltToSend = ( saltBalance * timeSinceLastUpkeep * rewardsConfig.emissionsWeeklyPercentTimes1000() ) / ( 100 * 1000 weeks );

Assume emissionsWeeklyPercentTimes1000 is 500, since timeSinceLastUpkeep is not controllable (max of one week), the saltBalance will eventually be a dust amount in the contract. Then, the saltToSend will truncate to zero.

uint256 saltToSend = ( saltBalance * timeSinceLastUpkeep * rewardsConfig.emissionsWeeklyPercentTimes1000() ) / ( 100 * 1000 weeks );

// Left 100 wei saltBalance
uint256 saltToSend = 100 * 604800 * 500 / 100 * 1000 * 604800 = 0

At the very end, make sure the salt amount can be withdrawn.

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/Emissions.sol#L52-L60

[L-02] In ManagedWallet.sol, the activeTimelock can be set even before proposeWallets is called, so the activeTimelock can be bypassed.

The confirmation wallet can send 0.05 ether even before proposeWallets() function is called to skip the TIMELOCK_DURATION.

    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 )
                //@audit - Skip this part by depositing 0.05 ether before calling `proposeWallets()`
                activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
        else
                        activeTimelock = type(uint256).max; // effectively never
        }

Make sure the activeTimelock is only set once the proposeWallet() function is called.

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

[L-03] In ManagedWallet.sol, the activeTimelock can be nullified at any time

If the confirmation wallet user changes his mind, he can send 1 wei of ether to the contract and reset the activeTimelock.

        if ( msg.value >= .05 ether )
>               activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
        else
>                       activeTimelock = type(uint256).max; // effectively never

This should not be the case, as once the activeTimelock is set, it should not be able to be nullified.

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

[L-04] In PriceAggregator.sol, if owner sets the priceFeed1 as the zero address, then he can set the price feeds again, which bypasses the intended restriction.

In setInitialFeeds, the owner can set _priceFeed1 as the zero address. He can then call setInitialFeeds() again

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

                //@audit - set _priceFeed1 as address(0)
                priceFeed1 = _priceFeed1;
                priceFeed2 = _priceFeed2;
                priceFeed3 = _priceFeed3;
                }

Have some checks in the setInitialFeeds() function like require(_priceFeed1 != address(0)) to prevent the owner from re-setting the initial feeds.

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

[L-05] Documentation and Code for Staking.sol does not match up

Document states:

Expedited unstaking is possible with a penalty - with the fastest default unstaking period being two weeks with a default penalty of 80%.

The fastest default unstaking period is actually 1 week, with a default penalty of 80%.

        function changeMinUnstakeWeeks(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (minUnstakeWeeks < 12)
                minUnstakeWeeks += 1;
            }
        else
            {
>           if (minUnstakeWeeks > 1)
                minUnstakeWeeks -= 1;
            }


                emit MinUnstakeWeeksChanged(minUnstakeWeeks);
        }

In my opinion, I think 1 week is better than 2 weeks because the penalty is already very heavy.

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingConfig.sol#L34-L48

[L-06] Users can just vote no in BootstrapBallot because they will be getting the airdrop either way

Users who have a signature (on the whitelist) can vote to start the exchange for the Salty protocol. Honestly, I don't see a reason why a vote is needed to start an exchange because if the vote is a resounding no, then will the whole contract be redeployed?

Also, voters that vote no are not penalized. They will still receive the same amount of airdrop as those who vote yes. So, there is not really a difference between a yes and no vote for the users perspective and the some griefers will vote no for fun because they will get the airdrop either way.

        function vote( bool voteStartExchangeYes, bytes calldata signature ) external nonReentrant
                {
                require( ! hasVoted[msg.sender], "User already voted" );


                // Verify the signature to confirm the user is authorized to vote
                bytes32 messageHash = keccak256(abi.encodePacked(block.chainid, msg.sender));
                require(SigningTools._verifySignature(messageHash, signature), "Incorrect BootstrapBallot.vote signatory" );


                if ( voteStartExchangeYes )
>                       startExchangeYes++;
                else
>                       startExchangeNo++;


                hasVoted[msg.sender] = true;


                // As the whitelisted user has retweeted the launch message and voted, they are authorized to the receive the airdrop.
>               airdrop.authorizeWallet(msg.sender);
                }

Consider implementing a small penalty for those that vote No. The current system is designed to assume the best of people, but I don't know if it's actually the ideal choice.

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

[L-07] Chainlink's latestRoundData has no check for round completeness and may return stale prices

When collecting data from Chainlink, the data returned is not sufficiently checked. Specifically, roundID is not checked, as well as the min-max threshold.

                try chainlinkFeed.latestRoundData()
                returns (
                        uint80, // _roundID
                        int256 _price,
                        uint256, // _startedAt
                        uint256 _answerTimestamp,
                        uint80 // _answeredInRound
                )

Implement the roundId check as well

require(answeredInRound >= roundID, "round not complete");

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

[L-08] CoreChainlinkFeed uses the BTC_USD address when it should be using the WBTC-BTC one as well

CoreChainlinkFeed assumes that the price of BTC is equal to the price of WBTC.

This is the BTC/USD feed:

https://data.chain.link/feeds/ethereum/mainnet/btc-usd

This is the WBTC/BTC feed:

https://data.chain.link/feeds/ethereum/mainnet/wbtc-btc

Note that the WBTC/BTC is not 1:1. Users may not get the actual WBTC price (since protocol is using WBTC instead of BTC) which will affect collaterization ratio and future liquidations.

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreChainlinkFeed.sol#L32-L39

[L-09] The 3 Oracles used in the protocol uses a different denominator for their price, which results in inconsistent price displayed

The protocol uses a price aggregator that aggregates prices from Chainlink, Salty and Uniswap.

The protocol intends to get the price of WBTC and WETH in terms of USD.

In CoreChainlinkFeed, the protocol gets the price of BTC in terms of USD.

        // https://docs.chain.link/data-feeds/price-feeds/addresses
        AggregatorV3Interface immutable public CHAINLINK_BTC_USD;
        AggregatorV3Interface immutable public CHAINLINK_ETH_USD;

In CoreSaltyFeed, the protocol gets the price of WBTC in terms of USDS.

        function getPriceBTC() external view returns (uint256)
                {
        (uint256 reservesWBTC, uint256 reservesUSDS) = pools.getPoolReserves(wbtc, usds);


                if ( ( reservesWBTC < PoolUtils.DUST ) || ( reservesUSDS < PoolUtils.DUST ) )
                        return 0;


                // reservesWBTC has 8 decimals, keep the 18 decimals of reservesUSDS
                return ( reservesUSDS * 10**8 ) / reservesWBTC;
                }

In CoreUniswapFeed, the protocol gets the price of WBTC in terms of USDC. (WBTC-WETH-USDC)

        constructor( IERC20 _wbtc, IERC20 _weth, IERC20 _usdc, address _UNISWAP_V3_WBTC_WETH, address _UNISWAP_V3_WETH_USDC )
                {
                UNISWAP_V3_WBTC_WETH = IUniswapV3Pool(_UNISWAP_V3_WBTC_WETH);
                UNISWAP_V3_WETH_USDC = IUniswapV3Pool(_UNISWAP_V3_WETH_USDC);

By doing so, the protocol assumes that USD,USDC and USDS all have the same price, and WBTC and BTC have the same price.

This is slightly inaccurate since they don't have the same price. WBTC has a different price from BTC.

Also, USDC has a different price from USD, that is why there is a Chainlink oracle for that:

https://data.chain.link/feeds/ethereum/mainnet/usdc-usd

Lastly, and most importantly, USDS may not be at $1 all the time. There are instances of a collateralized stablecoin depegging (USDR, DOLA, even DAI).

The most accurate feed would be the WBTC-USDC price from Uniswap TWAP. If the protocol wants to be more accurate in the pricing, consider adding a WBTC/BTC price for Chainlink and a USDC/USDS pool for SALT.

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L14-L17 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreSaltyFeed.sol https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreUniswapFeed.sol

[L-10] Protocol assumes that USDS will always be at $1.

When minting USDS and checking for liquidation, USDS will always be assumed to be $1.

This is my guess for the assumption: When the price of USDS is more than $1, users will deposit more collateral, mint USDS and swap them for USDC because they will earn money. This swap will eventually bring the price back down to a dollar.

When the price of USDS is less than $1, users will buy more USDS because it is cheaper to pay back their debt now, which will bring the price back up to a dollar.

While this assumption is true, it does not count for flash crashes. If WBTC and WETH happens to drop until most positions becomes undercollaterized, then the protocol will incur a lot of bad debt. In this sense, bad debt doesn't actually refer to a monetary amount, but it refers to USDS being depegged. When positions becomes undercollaterized and before liquidation, users will start selling their USDS instead of repaying their USDS. This will create a downward pressure and since the position is undercollaterized, even if WBTC and WETH is swapped for USDS and burned, it will not help to bring the price of USDS up much. Eventually, the price of USDS will fall and it's extremely hard to bring it back up to its peg.

The last option is to burn the Protocol owned liquidity in order to bring the price of USDS back up, but that might still not be sufficient.

Not sure about the actual recommendation, but one thing the protocol could do is use the USDC value for USDS instead of its own value. Also, it makes sense since the value of WETH and WBTC is calculated in USD/USDC and not USDS.

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

Approach taken when reviewing the protocol

This is a pretty large codebase, so I started with trying to understand the protocol first, before reading the code and getting the general idea of the protocol.

1. Watched the Video Technical Walkthrough to get a sense of the protocol.
2. Looked at the codes with the least amount of lines and started from there.
3. Started to understand the protocol a little more and grouped the contracts into different sections (Airdrop Section, Config Section, ERC20 Section, DEX Section, Lending Section, Staking Section)
4. Started linking the sections together and seeing how each contract affects another contract (eg how Airdrop section relates to Staking and Staking rewards)
5. Started understanding the Airdrop Section first, InitialDistribution, Airdrop and BootstrapBallot
6. Next, started figuring out the staking section. (SALT staking, xSALT staking)
7. Analyze the DAO and notice how the DAO has the capability of submitting proposals that affects every contract
8. After finishing the DAO, Parameters, Proposals and all the Config, went to look at Pools.
9. Compared the code of Pools to Uniswap V2 (similar idea), to see any difference between how both protocols tackle AMM functionality
10. Analyze the lending section (WBTC-WETH), Collaterization Ratio, Liquidation process (liquidizer)
11. Checked the PriceAggregators (Chainlink, Salt and Uniswap) and cross-compare to the whole lending process
12. Fill in the gaps (Arbitrage, Math)
13. Started end-to-end scenario analysis for hypothetical users (eg a user stakes 100 SALT. What happens? How much votes does he have?)
14. Looked through the test files.
15. Wrote the QA, Analysis and Report whilst going through the protocol

Summary and Mechanism Review

Airdropping tokens

User's POV:

1. The user must have a signature (be whitelisted or something else) and call BootstrapBallot.vote().
2. The user can vote yes or no to start the exchange
3. There is a time limit for the voting duration
4. Once the vote ends, if the vote passes, then users can go to claimAirdrop() and claim their SALT tokens
5. Note that the SALT cannot be claimed immediately, but is staked for the user. To claim the SALT directly, they have to call unstake and wait 52 weeks for the total duration.

• Mechanism is well thought out, users can only vote once and claim once.
• The amount of SALT distributed is distributed equally (5 million), and if there is low number of votes(), then authorized voters can get more SALT.
• It is uncertain how users can obtain a signature in order to be eligible to vote. If getting into the whitelist is easy, then users can create multiple accounts to vote (no) and grief the whole protocol (costs a lot of gas for them though)
• An important thing to note is that the vote can fail, and if the vote does fail, what happens next? Will the contract be redeployed?
• Also, if there are 0 authorized voters (extremely unlikely), then the whole process will fail.
• Contracts used: Airdrop.sol, BootstrapBallot.sol, InitialDistribution.sol, ExchangeConfig.sol, Staking.sol
• Mechanism is dependant on users calling vote and users actually voting yes.

Depositing Collateral and Minting USDS

User's POV:

1. The user deposits WBTC and WETH token into the Pools contract through CollateralAndLiquidity.depositCollateralAndIncreaseShare().
2. Users can call borrowUSDS() to mint USDS.
3. Users can repay their USDS debt by calling repayUSDS().

• A common collaterized stablecoin mechanism. Having an initial collateral ratio and minimum collateral ratio is good so that the users position will not get immediately liquidated when withdrawing the max amount of debt
• Initial Collateral ratio is set as 200%, with a 150-300% range. Mincollateral ratio is set at 110%, with a 110-120 range
• Users have to repay their USDS in order to withdraw their liquidity, which is correct.

Liquidation

Liquidator's POV:

1. Liquidator calls liquidateUser() in CollateralAndLiquidity.sol
2. If the user is primed for liquidation, all his WBTC and WETH tokens will be withdrawn from the pool.
3. The liquidator gets 5% of the WBTC and WETH tokens.
4. The WBTC and WETH tokens will be sent to the Liquidizer contract, get swapped to USDS and burnt
5. The liquidated user gets to keep his USDS debt.

• Interesting mechanism for liquidation. Normally, the liquidator will repay the debt, but in this case the liquidator simply calls a function.
• Unsure how burning the WBTC and WETH token will help the USDS token. Probably through the rebalancing of the pools?
• Liquidation rewards can range from 5-10%.

Depositing liquidity

User's POV:

1. User can deposit liquidity through Liquidity.depositLiquidityAndIncreaseShare(). Take note that users can only deposit into whitelisted pools and that the tokens are whitelisted.
2. User can withdraw liquidity through Liquidity.withdrawLiquidityAndClaim(). Note that there is a buffer time between depositing and withdrawing liquidity to prevent any flashloan rewards exploit.
3. When liquidity is withdrawn, SALT rewards will be distributed to the liquidity provider.

• There is no ERC20 liquidity token issued. Liquidity is just the combination of the amount of token A and token B, which get's pretty confusing when both tokens have different decimals.
• Users can only deposit an equal ratio of token A and token B, which is good to prevent any liquidity imbalance exploit.

Staking SALT

User's POV:

1. User calls stakeSALT() in the Staking.sol contract. The user's wallet must have access first
2. User shares is increased. When SALT is staked, it becomes xSALT. Users can use xSALT as voting power to ballot in proposals
3. User can unstake xSALT for SALT. The unstaking process is between 1 week to 1 year with a penalty of 80% for faster unstaking.
4. The SALT that is penalized from unstaking prematurely will be burned.
5. Users can cancel their unstake() at any point in time.

• Normal Staking mechanism, with penalties. Note that the user's votes still exist even when unstake is called.

Swapping Tokens

• Users can call depositSwapWithdraw() to swap tokens from a whitelisted pool (assuming it has enough liquidity)
• Instead of a typical AMM swap, it adjusts the reserves and attempt an arbitrage, which is a novel concept

Codebase quality analysis

PoolsConfig.sol

• The check for whitelist is from the increaseuserShare function. It seems that decreaseUserShare doesn't check for whitelist, which is correct.
• Since the owner is the only whitelisting the pool, issues with whitelisting pools will fall under centralization risks

CoreChainlinkFeed.sol

• A few Potential Issues
• The latestRoundData is not sufficiently checked, may return stale prices.
• MAX_ANSWER_DELAY is fixed, so make sure that BTC and ETH feeds also have the same delay
• Price returning as 0 instead of reverting can be dangerous. Not sure if it's the best for the protocol since it uses a price aggregator, if another price feed is manipulated then the zero return will be detrimental, however the scenario where 2 price feeds are down simultaneously is pretty unlikely
• Other than that, price return and use of latestRoundData is correct

DAOConfig.sol

• Same as other Config files, used in Parameter, which is inherited from DAO.sol. There are 8 changes here, so there must be 8 changes in the Parameter contract as well.

Proposals.sol

• Good that only one user can open one active proposal to prevent spamming
• The salt staking is quite interesting, because before any stake, a user can propose anything without much salt?
• It's a proposal phase so even though anyone can create a proposal, it's still a DAO effort to make sure it passes or not
• All the contracts call the proper channels
• Casting vote checks that the amount of votes can only be used once per ballot
• The required quorum checks for the votes percentage correctly.

Parameters.sol

• Make sure that StakingConfig has 4 types, DAOConfig has 8 types, rewardsConfig has 4 types, StableConfig has 6 types, PoolsConfig has 2 types, PriceAggregator has 2 types
• Function is written properly and all else if calls are correctly placed.
• All intended changes and increments are written correctly

Airdrop.sol

• The first thing that happens is that the bootstrap Ballot must authorize all the wallet it wants to authorize

• Once allow claiming is called (can only be called once), then there is no more authorized wallet.

• This means even before the salt is sent to the initial distribution contract, some wallets must already be authorized.

• Users must vote in order to be authorized in BootstrapBallot.sol

• For users, they have to vote first, then wait until initial distribution is called, then call claimAirdrop(). They can only claim once. Their tokens will pass through the staking contract and then be transferred to the user (must check this interaction)

• Is this authorization independent from the entire protocol? In other words, does it affect anything else?

• All functions are checked to be written correctly. Users that are not authorized cannot claim. Also, once initial distribution starts, no one can be authorized anymore.

BootstrapBallot.sol

• Only can vote once, but must call vote() to be authorized
• Potential Issue What if the majority votes to not start the exchange? what happens? The whole contract is redeployed? Also, is getting whitelisted easy? Can users just keep voting no with multiple accounts? (because they will still get airdrop anyways if the votes succeed)
• If succeed, will call initialdistribution.distributionApproved, and then airdrop.allowClaiming will be called. startExchangeApproved() will also be called.
• Voting and finalizing is done correctly. Pools and initial distribution does receive the approval

[InitialDistribution.sol]

• 52M to emissions, 25M to DAO vesting wallet, 10M to Team vesting wallet, 5M to airdrop participants, 5M to liquidity and 3M to staking, total to 100M.
• No salt should be left in this contract after distributionApproved is called.
• Once distributionApproved is called, no more wallets can be authorized. So these wallets may be on the whitelist but if they don't vote, they don't get the airdrop.
• This function assumes that the Ballot succeeds, and the WhitelistedPools is correct.
• No potential issues in this contract alone. MILLION_ETHER is written correctly, every calculation is multiplied properly. Check cannot be griefed because no other SALT can be minted other than the 100M given to the msg.sender of the SALT contract.

PriceAggregator.sol

• Potential Issue: in setInitialFeeds, if owner sets the first price feed1 as 0 address, then he can call setInitialFeeds again
• Not sure about the purpose of a priceFeedModificationCooldown since it's only changed if something serious is going to happen / has happened. It should be able to be changed immediately.
• Config files are set properly, with lower bound and upper bound tallying up with the comments
• Aggregate prices is written correctly. Even if 1 price feed returns 0, hopefully the other two price feeds returns the correct price, and gets the average sum
• The percentageDifference tolerance is about 3%, and be changed from 1% - 7%

CoreSaltyFeed.sol

• The reserve price cannot be less than DUST, which is 100.
• This contract interacts with pools.getPoolReserves, so must check whether that function returns the correct amount

RewardsConfig.sol

• Probably same as StakingConfig and the other Configs file, all change is called in Parameter.sol which is an abstract contract inherited by DAO.sol

Emissions.sol

• upkeep contract is set in the ExchangeConfig contract
• checked the calculation of saltToSend, seems correct

uint256 saltToSend = ( saltBalance * timeSinceLastUpkeep * rewardsConfig.emissionsWeeklyPercentTimes1000() ) / ( 100 * 1000 weeks );

Assume 100e18 as saltBalance, 0.5% should be 5e17.

(1 second)
100e18 * 1 * 500 / (100 * 1000 * 604800) = 826719576720

(1 week)
100e18 * 604800 * 500 / (100 * 1000 * 604800) = 5e17 (0.5%)

• This is called in the Upkeep contract, at step 6. It is assumed that the emission contract already has some salt balance? I think it's from initial distribution of 52 million salt.
• Potential Issue: Must check last emission, whether it will truncate to zero, apparently dust values would truncate.
• Emission calculation is checked, timeSinceLastUpkeep is checked, max of 1 week.

USDS.sol

• Inherits ERC20, Ownable
• Inheritance is correct, the minting from collateral contract is also correct.

StakingConfig.sol

• All the change is called in Parameter.sol, which is an abstract contract inherited from the DAO
• Only DAO can call these changes, which is changed through a balloting process
• The increase and decrease can be quite inconvenient (If minUnstakeWeeks is currently at 1, then have to call 11 times to become 12), but it's probably a design decision.
• Potential Issue: changeminunstake weeks ranges from 1-12, but docs mention 2-12.
• All other comments are checked to be consistent with the code.

ManagedWallet.sol

• Interesting to see how the mainWallet only can call proposeWallet and the confirmation wallet only can set the activeTimelock.
• 3 potential issues:
• 1. The activeTimelock can be set even before proposeWallets is called, so the activeTimelock can be bypassed
• 2. The contract cannot draw out the ether, ether stuck in contract
• 3. Confirmation wallet can reject the whole proposal by either not calling the contract or just sending in 1 wei.

AccessManager.sol

• grantAccess / walletHasAccess is used by which contracts?

Salt.sol

• Inherits ERC20
• Mint is correct, mints to a msg.sender, which is supposed to be sent to the InitialDistribution.

Architecture Review

Design Patterns and Best Practices:

• Common design patterns are used, like onlyOwner, or checking for DAO access, or reentrancy guards
• Abstract contracts and Inheritance is used well.
• Nuances like checking for DUST amounts, having min-collateral and initial-collateral, having a time delay for staking and unstaking shows that the protocol is written well.

Code Readability and Maintainability:

• Code is quite difficult to read due to the sheer size of the codebase and the amount of Math involved in AMM.
• Code also has a lot of external calls to other contracts which is quite confusing, but there is a common pattern of using Config and Parameters
• There is also a lot of internal calls, but that is to be expected from a large codebase with many different function

Error Handling and Input Validation:

• Events and Error messages are easy to understand.
• Input is validated well in every config file, and the code matches the comments.

Interoperability and Standards Compliance:

• Good knowledge of the ERC20 standard when creating USDS and SALT tokens.

Testing and Documentation:

• Extensive tests (unit, integration tests) done, reaching an overall coverage of almost 100%.
• Documentation (whitepaper, github) is plentiful and includes quality reasoning at every juncture of the protocol.
• One improvement could be having more diagrams and a summarized version of how the protocol works from the top down, with a end-to-end scenario of how a user can interact with the protocol (what function should the user call first, what can a user accomplish, why is the user incentivized to use the protocol)

Upgradeability:

• Protocol is not intended to be upgradeable, but contracts can be redeployed and rerouted easily through the changeManagers and changeRegistries function.

Dependency Management:

Protocol relies on external libraries like OpenZeppelin. Protocol should keep an eye on vulnerabilities that affects those external integrations, and make changes where necessary.

Overall, great architecture from the protocol, slight changes would be to the written code itself (using modifiers for repeated code, checking zero values, checking overflows etc) and more real life scenarios in the documentation.

Centralization Risks

Not much centralization risks as the protocol is almost run fully on the DAO and votes. The only thing the DAO can do is control which proposals to be passed by finalizing the Ballot. Otherwise, most of the protocol functions like clockwork.

Time spent:

40 hours