Salty.IO - lsaudit &#x27;s results

File: InitialDistribution

		// Send 10% of the rewards to the initial team
		uint256 amountToSendToTeam = claimedSALT / 10;

The percentage of rewards which will be sent to the initial team is set to 10% and hard-coded into the contract. It's not possible to change it in the future. Consider implementing a way of changing this value in the future and do not hard-code it in the protocol.

[2] Mixing tabs and white-space in code-formatting

Multiple of contracts does not obey the best coding standard in the context of tabulations and white-spaces. Tabs and white-spaces are mixed. This issue occurs basically in every contract. For the QA report readability, we've marked down only two examples of this issue. Nonetheless, this issue affects every contract in scope.

    	require( msg.sender == address(bootstrapBallot), "InitialDistribution.distributionApproved can only be called from the BootstrapBallot contract" );  // @audit: 4 whitespaces + tab
		require( salt.balanceOf(address(this)) == 100 * MILLION_ETHER, "SALT has already been sent from the contract" ); // // @audit: 2 tabs

File: USDS.sol

	constructor()
	ERC20( "USDS", "USDS" )
		{ // @audit: 2 tabs
        }  // @audit: 8 whitespaces

File: CollateralAndLiquidity.sol

	using SafeERC20 for IERC20;  // @audit: 1 tab
	using SafeERC20 for IUSDS; // @audit: 1 tab
    using EnumerableSet for EnumerableSet.AddressSet; // @audit: 4 whitespaces

    IStableConfig immutable public stableConfig; // @audit: 4 whitespaces
	IPriceAggregator immutable public priceAggregator; // @audit: 1 tab
    IUSDS immutable public usds; // @audit: 4 whitespaces

[3] The same wallet can be authorized multiple of times in `Airdrop.sol`

Function authorizeWallet() utilizez OZ's EnumerableSet implementation. If the set already contains the value which protocol tries to add - function add() will return false (instead of reverting). This implies that function authorizeWallet() from Airdrop.sol can be called multiple of times, with the same wallet address. Basically, it won't pose any security risk (since the wallet would had already been added to _authorizedUsers) - thus this issue is only reported in the QA report.

A good security practice is to verify if the value is already in set and revert if it is. This could be achieved by changing below code:

File: Airdrop.sol

_authorizedUsers.add(wallet);

to:

require(_authorizedUsers.add(wallet), "already added");

[4] Function `vote()` does not verify the contract address, meaning it might be possible to vote multiple of times on the same chain

		bytes32 messageHash = keccak256(abi.encodePacked(block.chainid, msg.sender));
		require(SigningTools._verifySignature(messageHash, signature), "Incorrect BootstrapBallot.vote signatory" );

The messageHash contains only block.chanid and msg.sender. If the contract would be re-deployed to the different address (or when someone else would decide to use the fork of the protocol's implementation and deploy it to the different address) - then it would be possible to re-use messageHash. It's good security practice to utilize address(this) when calculating messageHash, so that it won't be possible to re-use the same messageHash on the different contract addresses.

[5] Lack of address(0) check in `setPriceFeed()` in `PriceAggregator.sol`

		if ( priceFeedNum == 1 )
			priceFeed1 = newPriceFeed;
		else if ( priceFeedNum == 2 )
			priceFeed2 = newPriceFeed;
		else if ( priceFeedNum == 3 )
			priceFeed3 = newPriceFeed;

Function setPriceFeed() does not perform any validation for address(0). Whenever newPriceFeed parameter is passed as address(0), function will set priceFeed1 (or priceFeed2, priceFeed3) to address(0). Since this function is called by only owner (onlyOwner modifier), the severity has been reduced to Low and included in QA report. Nonetheless, it's good practice to verify if address is not address(0), whenever we set new price feed.

Our recommendation is to add additional check:

require(newPriceFeed != address(0), "new price feed cannot be address(0)");

[6] Setters does not verify if the value was really changed

File: ExchangeConfig.sol

	function setAccessManager( IAccessManager _accessManager ) external onlyOwner
		{
		require( address(_accessManager) != address(0), "_accessManager cannot be address(0)" );

		accessManager = _accessManager;

	    emit AccessManagerSet(_accessManager);
		}

Whenever we update/set a new value of some state variable, it's a good practice to make sure that, that value is being indeed updated. In the current implementation of setAccessManager(), even when the _accessManager won't be changed, function will still emit an AccessManagerSet() event - which might be misleading to the end-user. E.g. let's assume that _accessManager = 0x123. Calling setAccessManager(0x123) won't update/set anything new, but AccessManagerSet() will still be called.

Our recommendation is to add additional require check, to verify, that provided values are indeed different than the current ones:

require(address(_accessManager) != address(accessManager ), "Nothing to update");

[7] Dust should be linked to tokens based on their decimals

File: PoolUtils.sol

	// Token reserves less than dust are treated as if they don't exist at all.
	// With the 18 decimals that are used for most tokens, DUST has a value of 0.0000000000000001
	uint256 constant public DUST = 100;

The DUST value is set to 100. While this is a very small amount of 18 decimals tokens, it might not be good enough for tokens with smaller decimals amount. Protocol does not explicitly states what kind of tokens will be used. There's no assumptions that tokens with, e.g. 8 decimals won't be used. For that tokens, the DUST will be defined as 1e-06. This is still small amount, but, it's extensively higher than 1e-16. We did not report this issue as Medium, because we believe that it's up to developers to choose if 1e-6 dust for 8 decimals token is acceptable amount of DUST. Nonetheless, this problem could be resolved by having additional mapping, which links the tokens to the acceptable DUST amount for that token. E.g. for tokens with 8 decimals, DUST can be smaller.

[8] `constructor()` in `ManagedWallet.sol` allows to set wallets' addresses to `address(0)`.

Since this issue occurs in the constructor(), the severity of this issue was evaluated as Low, thus this issue is being reported in the QA report.

File: ManagedWallet.sol

	constructor( address _mainWallet, address _confirmationWallet)
		{
		mainWallet = _mainWallet;
		confirmationWallet = _confirmationWallet;

Both mainWallet and confirmationWallet can be set to address(0), since constructor() does not verify if parameters _mainWallet and _confirmationWallet are not address(0).

[9] It's possible to call `setContracts()` in `ExchangeCOnfig.sol` multiple of times in some rare conditions

File: ExchangeConfig.sol

// setContracts can only be be called once - and is called at deployment time.
	function setContracts( IDAO _dao, IUpkeep _upkeep, IInitialDistribution _initialDistribution, IAirdrop _airdrop, VestingWallet _teamVestingWallet, VestingWallet _daoVestingWallet ) external onlyOwner
		{
		// setContracts is only called once (on deployment)
		require( address(dao) == address(0), "setContracts can only be called once" );

		dao = _dao;

According to the comments, function setContracts() should be called only once. This invariant, is however broken, when _dao is set to address(0).

Function setContracts() does not check if parameter _dao is not address(0). This means, that it's possible to call setContracts() multiple of times by passing _dao as address(0).

When function setContracts() is called for the first time with _dao set to address(0) - dao will still point out to address(0). This means, that it would be possible to call this function once again - even though, it should not be possible.

Due to very rare likelihood (only owner needs to call setContracts() with _dao set to address(0)) - this issue has been reported in QA reports only as bad coding practice.

Nonetheless, our recommendation is to add additional check which requires that _dao cannot be address(0): require(_dao != address(0), "dao cannot be addr(0)").

[10] Typo in event name

Event names should start with upper-case letters.

event maximumInternalSwapPercentTimes1000Changed(uint256 newMaxSwap);

maximumInternalSwapPercentTimes1000Changed should be changed to MaximumInternalSwapPercentTimes1000Changed.

[11] Use predefined constants in comments

// Returns uint64.max in the event of failed lookup

The code uses previously defined constant INVALID_POOL_ID. It will be much clearer to use this constant in the comment section, instead of the hard-coded value:

// Returns INVALID_POOL_ID (uint64.max) in the event of failed lookup

[12] Incorrect comment in `Pools.sol`

		// Make sure that the reserves after swap are above DUST
        require( (reserve0 >= PoolUtils.DUST) && (reserve1 >= PoolUtils.DUST), "Insufficient reserves after swap");

Since >= operator is being used, the comment should be: Make sure that the reserves after swap are not below DUST.

[13] Stick to one coding-style during function calls

Good coding style defines calling function syntax as: functionName(parameter). However, across multiple of contracts, we have spot a redundant white-space before and/or after the parentheses: functionName( parameter), functionName(parameter ), functionName( parameter ). It's a good coding practice to stick to one style in the syntax.

Across every contract, multiple of function calls are being used. E.g., in multiple of places, there's a whitespace after opening parentheses, while in other - the whitespace is missing: A(test), A( test ). Having different coding-styles decreases code readability.

This is an overall issue across every contract. Since this is juts a minor issue which does not affect the contract's security at all (it affects only the code quality) - for the report's clarity, we have decided to point out only one example of this issue. Please keep in mid that this problem occurs basically in every contract.

332: (uint256 reservesA0, uint256 reservesA1) = getPoolReserves( weth, arbToken2);  // @audit: A( test)
350: swapAmountOut = _adjustReservesForSwap( swapTokenIn, swapTokenOut, swapAmountIn ); // @audit: A( test )
411: (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB); // @audit: A(test)

[14] Use white-space in mappings' syntax

Using white-spaces before and after => in the mappings improves the code readability.

mapping(IERC20=>uint256) storage userDeposits = _userDeposits[msg.sender];

should be changed to:

mapping(IERC20 => uint256) storage userDeposits = _userDeposits[msg.sender];

Other instances of this issue:

./src/dao/DAO.sol:67:   mapping(string=>bool) public excludedCountries;
./src/dao/Proposals.sol:38:     mapping(string=>uint256) public openBallotsByName;
./src/dao/Proposals.sol:41:     mapping(uint256=>Ballot) public ballots;
./src/dao/Proposals.sol:51:     mapping(uint256=>mapping(Vote=>uint256)) private _votesCastForBallot;
./src/dao/Proposals.sol:55:     mapping(uint256=>mapping(address=>UserVote)) private _lastUserVoteForBallot;
./src/dao/Proposals.sol:59:     mapping(address=>bool) private _userHasActiveProposal;
./src/dao/Proposals.sol:63:     mapping(uint256=>address) private _usersThatProposedBallots;
./src/dao/Proposals.sol:344:            mapping(Vote=>uint256) storage votes = _votesCastForBallot[ballotID];
./src/dao/Proposals.sol:357:            mapping(Vote=>uint256) storage votes = _votesCastForBallot[ballotID];
./src/dao/Proposals.sol:366:            mapping(Vote=>uint256) storage votes = _votesCastForBallot[ballotID];
./src/launch/Airdrop.sol:29:    mapping(address=>bool) public claimed;
./src/launch/BootstrapBallot.sol:25:    mapping(address=>bool) public hasVoted;
./src/pools/Pools.sol:46:       mapping(bytes32=>PoolReserves) private _poolReserves;
./src/pools/Pools.sol:49:       mapping(address=>mapping(IERC20=>uint256)) private _userDeposits;
./src/pools/Pools.sol:369:              mapping(IERC20=>uint256) storage userDeposits = _userDeposits[msg.sender];
./src/pools/PoolsConfig.sol:32: mapping(bytes32=>TokenPair) public underlyingPoolTokens;
./src/pools/PoolStats.sol:21:   mapping(bytes32=>uint256) public _arbitrageProfits;
./src/pools/PoolStats.sol:24:   mapping(bytes32=>ArbitrageIndicies) public _arbitrageIndicies;
./src/rewards/RewardsEmitter.sol:35:    mapping(bytes32=>uint256) public pendingRewards;
./src/stable/CollateralAndLiquidity.sol:49:    mapping(address=>uint256) public usdsBorrowedByUsers;
./src/staking/Staking.sol:29:    mapping(uint256=>Unstake) private _unstakesByID;
./src/staking/StakingRewards.sol:36:    mapping(address=>mapping(bytes32=>UserShareInfo)) private _userShareInfo;
./src/staking/StakingRewards.sol:39:    mapping(bytes32=>uint256) public totalRewards;
./src/staking/StakingRewards.sol:42:    mapping(bytes32=>uint256) public totalShares;
./src/staking/StakingRewards.sol:149:           mapping(bytes32=>UserShareInfo) storage userInfo = _userShareInfo[msg.sender];
./src/staking/StakingRewards.sol:294:           mapping(bytes32=>UserShareInfo) storage userInfo = _userShareInfo[wallet];

[15] Document constant numbers used in calculations

if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )

Above code does not contain any comment which explains why we're multiplying by 100000. Since this is a percent and Times1000, we should multiply by 100 * 1000, which is indeed 100000. However, using this number without any explanation in the code-base might be very misleading. Our recommendation is to document every non-obvious constant-numbers.

[16] Functions from `*Config.sol` emits event even when nothing is changed

A lot of functions in *Config.sol files implement functions which are defined as below:

if (increase) 
{
	if (check if in range) {
		do something
	}
}
else {
	if (check if in range) {
		do something
	}
}

emit someEvent();

This basically means that emit is emitted no matter if the action was taken or not. E.g., let's take a look at changeBallotDuration() function:

File: DAOConfig.sol

	function changeBallotDuration(bool increase) external onlyOwner
    	{
        if (increase)
        	{
            if (ballotMinimumDuration < 14 days)
                ballotMinimumDuration += 1 days;
        	}
        else
        	{
            if (ballotMinimumDuration > 3 days)
                ballotMinimumDuration -= 1 days;
        	}

		emit BallotDurationChanged(ballotMinimumDuration);
    	}

When ballotMinimumDuration is set to 14 days (we would need to call changeBallotDuration(true) a few times to achieve this state), then calling changeBallotDuration(true) one more time won't update ballotMinimumDuration (because 14 < 14 is false and we won't enter conditional branch at line 119). Nonetheless, the BallotDurationChanged() event will still be emitted.

When nothing is changed, function should not emit an event. The event should be moved to the if condition, so that it will be only executed when some configuration would be updated.

The same issue occurs in almost every functions in every *Config.sol files:

DAOConfig.sol Functions: changeBootstrappingRewards(), changePercentPolRewardsBurned(), changeBaseBallotQuorumPercent(), changeBallotDuration(), changeRequiredProposalPercentStake(), changeMaxPendingTokensForWhitelisting(), changeArbitrageProfitsPercentPOL(), changeUpkeepRewardPercent()
StableConfig.sol Functions: changeRewardPercentForCallingLiquidation(), changeMaxRewardValueForCallingLiquidation(), changeMinimumCollateralValueForBorrowing(), changeInitialCollateralRatioPercent(), changeMinimumCollateralRatioPercent(), changePercentArbitrageProfitsForStablePOL()
PoolsConfig.sol Functions: changeMaximumWhitelistedPools(), changeMaximumInternalSwapPercentTimes1000()
RewardsConfig.sol Functions: changeRewardsEmitterDailyPercent(), changeEmissionsWeeklyPercent(), changeStakingRewardsPercent(), changePercentRewardsSaltUSDS()
StakingConfig.sol Functions: changeMinUnstakeWeeks(), changeMaxUnstakeWeeks(), changeMinUnstakePercent(), changeModificationCooldown()

The same issue has been identified also in PriceAggregator.sol: Functions: changeMaximumPriceFeedPercentDifferenceTimes1000(), changePriceFeedModificationCooldown().

[17] Lack of data verification in `setPriceFeed()` in `PriceAggregator.sol`

		if ( priceFeedNum == 1 )
			priceFeed1 = newPriceFeed;
		else if ( priceFeedNum == 2 )
			priceFeed2 = newPriceFeed;
		else if ( priceFeedNum == 3 )
			priceFeed3 = newPriceFeed;

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

Function setPriceFeed() does not validate priceFeedNum parameter. Since this function is called by only owner (onlyOwner modifier), the severity has been reduced to Low and included in QA report.

This parameter is responsible for setting a price feed (when it's 1 - function updates priceFeed1, when it's 2 - it updates priceFeed2, when it's 3 - it updates priceFeed3). However, when priceFeedNum > 3 or priceFeedNum == 0, function won't update any price feed, but it will still emit PriceFeedSet() event. This behavior may be misleading to the end user.

Our recommendation is to revert when priceFeedNum > 3 or priceFeedNum == 0: require(priceFeedNum <= 3 && priceFeedNum > 0, "incorrect price feed");

[18] Standarize how `if` conditions are coded

There are two ways of coding if, when it contains a single instruction:

with brackets: if() { doSomething(); }
without brackets: if() doSomething();

The first way increases the code readability and is recommended. This is extremely important when two if conditions are coded in a row, e.g.:

		if ( useCooldown )
		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
			{
			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );
			[...]
			}

Having two if might be confusing to the code-reader. Our recommendation is to use {} and proper indentation in that case:

		if ( useCooldown )
		{
			if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
			{
			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );
			[...]
			}
		}

The same issue occurs in lines 104-105:

		if ( useCooldown )
		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown

[19] Incorrect grammar

// The ballot is approved. Any reversions below will allow the ballot to be attemped to be finalized later - as the ballot won't be finalized on reversion.

attemped should be changed to attempt.

File: Liquidity.sol

// The added liquidity will be owned by this contract. (external call to Pools contract)

Start the new sentence (after the dot) with upper-cased letter:

// The added liquidity will be owned by this contract. (External call to Pools contract)

[20] Use `&&` instead of two `if`-conditions in a row in `Proposals.sol`

		if ( increaseTotal > decreaseTotal )
		if ( increaseTotal > noChangeTotal )
			return Vote.INCREASE;

		if ( decreaseTotal > increaseTotal )
		if ( decreaseTotal > noChangeTotal )
			return Vote.DECREASE;

Using two if-conditions in a row - without curly brackets ({}) decreases the code readability. It will be much better to use && operator, since those conditions are basically the same:

		if ( increaseTotal > decreaseTotal && increaseTotal > noChangeTotal)
			return Vote.INCREASE;

		if ( decreaseTotal > increaseTotal && decreaseTotal > noChangeTotal)
			return Vote.DECREASE;

[21] Incorrect punctuation in comments

File: Liquidity.sol

// Cooldown is specified to prevent reward hunting (ie - quickly [...]

should be changed to: // Cooldown is specified to prevent reward hunting (ie., - quickly [...]

[22] It's possible to call `setInitialFeeds()` in `PriceAggregator.sol` multiple of times in some rare conditions

		require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );

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

Function setInitialFeeds() should be called only once. This invariant, is however broken, when _priceFeed1 is set to address(0).

Function setInitialFeeds() does not check if parameter _priceFeed1 is not address(0). This means, that it's possible to call setInitialFeeds() multiple of times by passing _priceFeed1 as address(0).

When function setInitialFeeds() is called for the first time with _priceFeed1 set to address(0) - priceFeed1 will still point out to address(0). This means, that it would be possible to call this function once again - even though, it should not be possible.

Please notice that it's possible to set priceFeed2 and priceFeed3 multiple of times, when _priceFeed1 will always be set to address(0).

E.g.

setInitialFeeds(address(0), address(0xAAA), address(0xBBB))

After the first call, priceFeed2 and priceFeed3 will be set to 0xAAA and 0xBBB. It should not be possible to call setInitialFeeds() again, but it's still possible:

setInitialFeeds(address(0), address(0x123), address(0x456))

After the 2nd call, priceFeed2 and priceFeed3 will be set to 0x123, 0x456.

Due to very rare likelihood (only owner needs to call setInitialFeeds() with _priceFeed1 set to address(0)) - this issue has been reported in QA reports only as bad coding practice.

Nonetheless, our recommendation is to add additional check which requires that _priceFeed1 cannot be address(0): require(_priceFeed1 != address(0), "price feed cannot be addr(0)").

[23] Off-by-one error when checking if quorum is reached

Function requiredQuorumForBallotType() defines how much votes we need to reach the quorum. It's being used in winningParameterVote() to verify if quorum is reached:

        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;

Reaching the quorum - by definition - means that the proposal has at least the same or more votes than the quorum defines. However, using < instead of <= operator means that proposal needs to exceed the defined quorum to be accepted.

E.g. if quorum is defined as 100 (function requiredQuorumForBallotType() returns 100), then when totalVotesCastForBallot(ballotID) returns 100, the quorum should be reached (we had defined quorum to 100, thus when there are 100 votes, quorum is reached). However, in Proposals.sol contract, (because of the < operator), function totalVotesCastForBallot(ballotID) needs to return 101 - to make the quorum be reached. This is an off-by-one issue related to the quorum definition. Even though it does not have any security related implications - our recommendation is to change < to <= operator.

[24] `winningParameterVote()` from `Proposals.sol` may return inconsistent result

if ( increaseTotal > decreaseTotal )
		if ( increaseTotal > noChangeTotal )
			return Vote.INCREASE;

		if ( decreaseTotal > increaseTotal )
		if ( decreaseTotal > noChangeTotal )
			return Vote.DECREASE;

		return Vote.NO_CHANGE;

Above conditions does not correctly choose the winning parameter vote when there's a tie. Let's consider 4 possible scenarios:

votes[Vote.INCREASE]  = 10000
votes[Vote.DECREASE]  = 10000
votes[Vote.NO_CHANGE] = 10000

Since Vote.INCREASE == Vote.DECREASE == Vote.NO_CHANGE function will return Vote.NO_CHANGE. This is reasonable, since there are equally the same votes for each options - it's ambiguous what do to - thus no change.

votes[Vote.INCREASE]  = 1
votes[Vote.DECREASE]  = 10000
votes[Vote.NO_CHANGE] = 10000

Since Vote.INCREASE == Vote.Vote.NO_CHANGE and there's is very small amount of votes for Vote.INCREASE, function will return Vote.NO_CHANGE. This is reasonable. Since almost no one voted for Vote.INCREASE and there's a tie between Vote.DECREASE and Vote.NO_CHANGE - thus no change.

votes[Vote.INCREASE]  = 10000
votes[Vote.DECREASE]  = 1
votes[Vote.NO_CHANGE] = 10000

The same reasoning as above.

votes[Vote.INCREASE]  = 10000
votes[Vote.DECREASE]  = 10000
votes[Vote.NO_CHANGE] = 1

Here is, however, where the result is incorrect. Let's check the exact flow:

if ( increaseTotal > decreaseTotal ) if ( increaseTotal > noChangeTotal ) is false. if ( decreaseTotal > increaseTotal ) if ( decreaseTotal > noChangeTotal ) is false. Thus function returns Vote.NO_CHANGE. This is, however, incorrect. Please notice, that multiple of people voted on either Vote.INCREASE or Vote.DECREASE and almost no one on Vote.NO_CHANGE - but still, the winning is Vote.NO_CHANGE. Consider having additional rule which will prioritize either Vote.INCREASE or Vote.DECREASE in case of tie, when Vote.DECREASE == Vote.INCREASE and Vote.NO_CHANGE is very small, function should return either Vote.DECREASE or Vote.INCREASE.

[25] The same pool can be whitelisted multiple of times in `PoolsConfig.sol`

Function whitelistPool() utilizes OZ's EnumerableSet implementation. If the set already contains the value which protocol tries to add - function add() will return false (instead of reverting). This implies that function whitelistPool() from PoolsConfig.sol can be called multiple of times, with the same pool. Basically, it won't pose any security risk (since the pool would had already been added to _whitelist) - thus this issue is only reported in the QA report.

A good security practice is to verify if the value is already in set and revert if it is. This could be achieved by changing below code:

_whitelist.add(poolID);

to:

require(_whitelist.add(poolID), "already added");

[26] It's possible to unwhitelist non-whitelisted pool

Function unwhitelistPool() utilizes OZ's EnumerableSet implementation. If the set does not contain the value which protocol tries to remove - function remove() will return false (instead of reverting). This implies that function unwhitelistPool() from PoolsConfig.sol can be called multiple of times, with the same pool or can be called on the pool which hadn't been whitelisted. During the every call (even for non-whitelisted pools), function will emit PoolUnwhitelisted() event. This behavior may be very misleading to the end-user. If pool does not exist and/or is not whitelisted, calling unWhitelistPool() should revert, instead of emitting PoolUnwhitelisted() event

A good security practice is to verify if the value is already in set and revert if we're trying to remove the element which does not exist. This could be achieved by changing below code:

_whitelist.remove(poolID);

to:

require(_whitelist.remove(poolID), "pool not whitelisted");

[27] Unclear comment in `PoolMath.sol`

File: PoolMath.sol

		// Here for reference
//        uint256 C = r0 * ( r0 * z1 - r1 * z0 ) / ( r1 + z1 );
//        uint256 discriminant = B * B - 4 * A * C;

		// Negate C (from above) and add instead of subtract.
		// r1 * z0 guaranteed to be greater than r0 * z1 per the conditional check in _determineZapSwapAmount
        uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
        uint256 discriminant = B * B + 4 * A * C;

Comment states that: // uint256 discriminant = B * B - 4 * A * C; (line 188) However, the code at line 193 is defined as below: uint256 discriminant = B * B + 4 * A * C;. As demonstrated, we're adding, instead of subtracting 4 * A * C. However, when we take a deeper look at another comment, at line 190: // Negate C (from above) and add instead of subtract., we can see, that indeed, we need to add, instead of subtract. The structure of the comment is, however, extremely misleading. When some piece of comment references to the exact math calculation, it should fully explain the code in the same comment. Do not use two different comments to describe the same calculation, especially, when those comments are contradicting themselves. The much better idea would be to comment above math as a single comment:

		// Here for reference 
//        uint256 C = r0 * ( r0 * z1 - r1 * z0 ) / ( r1 + z1 );
//        uint256 discriminant = B * B + 4 * A * C; /// C is negated, thus add instead of subtract

[28] Redundant calculations in `RewardsEmitter.sol`

File: RewardsEmitter.sol

115: 	uint256 sum = 0;
116:	for( uint256 i = 0; i < poolIDs.length; i++ )
[...]
128: 			sum += amountToAddForPool;

Function performUpkeep() - at line 115 - initializes variable sum and then, in every loop iteration - it increases its value by amountToAddForPool - line 128. However, the further code analysis demonstrated, that this variable is nowhere used. Initialization of this variable (line 115) and adding amountToAddForPool to it (line 128) is redundant, because performUpkeep() does not utilize this variable at all. Removing an unused variables and unused functionality of the code (keeping the sum of amountToAddForPool in sum variable) increases the code readability. Our recommendation is to remove line 115 and 128 from the performUpkeep().

[29] Typos

File: SigningTools.sol

// Verify that the messageHash was signed by the authoratative signer.

authoratative should be changed to authoritative.

[30] Amount required to confirm wallet is hardcoded in the contract, thus it's not possible to change it in the future

File: ManagedWallet.sol

    	if ( msg.value >= .05 ether )
    		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock

Value is hardcoded to 0.05 ether. If ETH value increases in its value in the future, 0.05 might be too expensive value to pay to confirm a wallet. Our recommendation is to implement a way to update this value in the future and do not hard-code it directly into the code.

[31] Standardize syntax for `for`-loops

Good coding practice requires to use white-space between for keyword and parenthesis. This is done only in CollateralAndLiquidity.sol

./src/stable/CollateralAndLiquidity.sol:321:                    for ( uint256 i = startIndex; i <= endIndex; i++ )
./src/stable/CollateralAndLiquidity.sol:338:            for ( uint256 i = 0; i < count; i++ )

The rest of the code-base uses for( instead of for (.

./src/arbitrage/ArbitrageSearch.sol:115:                        for( uint256 i = 0; i < 8; i++ )
./src/dao/Proposals.sol:423:            for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
./src/pools/PoolStats.sol:53:           for( uint256 i = 0; i < poolIDs.length; i++ )
./src/pools/PoolStats.sol:65:           for( uint256 i = 0; i < poolIDs.length; i++ )
./src/pools/PoolStats.sol:81:           for( uint256 i = 0; i < poolIDs.length; i++ )
./src/pools/PoolStats.sol:106:          for( uint256 i = 0; i < poolIDs.length; i++ )
./src/rewards/RewardsEmitter.sol:60:            for( uint256 i = 0; i < addedRewards.length; i++ )
./src/rewards/RewardsEmitter.sol:116:           for( uint256 i = 0; i < poolIDs.length; i++ )
./src/rewards/RewardsEmitter.sol:146:           for( uint256 i = 0; i < rewards.length; i++ )
./src/rewards/SaltRewards.sol:64:               for( uint256 i = 0; i < addedRewards.length; i++ )
./src/rewards/SaltRewards.sol:87:               for( uint256 i = 0; i < addedRewards.length; i++ )
./src/rewards/SaltRewards.sol:129:              for( uint256 i = 0; i < poolIDs.length; i++ )
./src/staking/Staking.sol:163:        for(uint256 i = start; i <= end; i++)
./src/staking/StakingRewards.sol:152:           for( uint256 i = 0; i < poolIDs.length; i++ )
./src/staking/StakingRewards.sol:185:           for( uint256 i = 0; i < addedRewards.length; i++ )
./src/staking/StakingRewards.sol:216:           for( uint256 i = 0; i < shares.length; i++ )
./src/staking/StakingRewards.sol:226:           for( uint256 i = 0; i < rewards.length; i++ )
./src/staking/StakingRewards.sol:260:           for( uint256 i = 0; i < rewards.length; i++ )
./src/staking/StakingRewards.sol:277:           for( uint256 i = 0; i < shares.length; i++ )
./src/staking/StakingRewards.sol:296:           for( uint256 i = 0; i < cooldowns.length; i++ )

[32] Use `uint256` instead of `uint`

While uint is a shorter form of uint256, using the latter increases the code readability:

Almost the whole implementation uses uint256. The shorter form was noticed only in two places:

./src/pools/Pools.sol:81:       modifier ensureNotExpired(uint deadline)
./src/staking/Liquidity.sol:40: modifier ensureNotExpired(uint deadline)

[33] Lack of min/max duration for ballots

	constructor( IExchangeConfig _exchangeConfig, IAirdrop _airdrop, uint256 ballotDuration )
		{
		require( ballotDuration > 0, "ballotDuration cannot be zero" );

		exchangeConfig = _exchangeConfig;
		airdrop = _airdrop;

		completionTimestamp = block.timestamp + ballotDuration;
		}

In constructor(), parameter ballotDuration is responsible for setting the ballot duration. However, this parameter is nowhere validated. This implies, that it's possible to create a ballot voting which will end immediately (e.g. by setting ballotDuration to 1 ) or to create ballot voting which will be active for extremely long period of time (e.g. setting ballotDuration to very big number).

Our recommendation is to set some ballotDuration restrictions (some min. and max. voting duration).

#0 - c4-judge
2024-02-03T14:10:07Z

Picodes marked the issue as grade-a

#1 - c4-judge
2024-02-07T18:12:41Z

Picodes marked the issue as grade-b

#2 - c4-judge
2024-02-07T18:12:46Z

Picodes marked the issue as grade-a

#3 - c4-sponsor
2024-02-08T10:37:04Z

othernet-global (sponsor) confirmed

Findings Information

🌟 Selected for report: 0xVolcano

Also found by: 0x11singh99, 0xAnah, Beepidibop, JCK, JcFichtner, K42, Kaysoft, Pechenite, Raihan, Rolezn, dharma09, hunter_w3b, lsaudit, n0kto, naman1778, niroh, sivanesh_808, slvDev, unique

Awards

216.4912 USDC - $216.49

Labels

bug

G (Gas Optimization)

grade-a

sponsor acknowledged

G-15

External Links

Link to GitHub issue

[G-01] Claiming airdrops in `Airdrop.sol` can be fully rewritten and optimized

Airdrop.sol utilizes mapping claimed to check if user has already claimed some token:

File: Airdrop.sol

	// Those users who have already claimed
	mapping(address=>bool) public claimed;

Now, in function claimAirdrop(), we're checking this mapping (and update it):

File: Airdrop.sol

function claimAirdrop() external nonReentrant
    	{
    	require( claimingAllowed, "Claiming is not allowed yet" );
    	require( isAuthorized(msg.sender), "Wallet is not authorized for airdrop" );
    	require( ! claimed[msg.sender], "Wallet already claimed the airdrop" );

		// Have the Airdrop contract stake a specified amount of SALT and then transfer it to the user
		staking.stakeSALT( saltAmountForEachUser );
		staking.transferStakedSaltFromAirdropToUser( msg.sender, saltAmountForEachUser );

    	claimed[msg.sender] = true;
    	}

This is, however - redundant, since we already have a list of users who can claim airdrop in _authorizedUsers. When claiming is allowed, no other users can be added to _authorizedUsers (function authorizeWallet() does not allow to add new wallets when claiming is allowed). This constitutes a very simple and elegant solution. Whenever we call claimAirdrop() - we can just remove user from _authorizedUsers.

Please notice, that we already check if user is on _authorizedUsers: (line 77) require( isAuthorized(msg.sender), "Wallet is not authorized for airdrop" );. If after claiming an airdrop, we would remove it from _authorizedUsers, he won't be able to call claimAirdrop() again. Moreover, since _authorizedUsers is Open Zeppelin's enumerable set, removing element from that set costs O(1).

To sum up, function claimAirdrop() can be rewritten:

function claimAirdrop() external nonReentrant
    	{
    	require( claimingAllowed, "Claiming is not allowed yet" );
    	require( isAuthorized(msg.sender), "Wallet is not authorized for airdrop" );
    	

		// Have the Airdrop contract stake a specified amount of SALT and then transfer it to the user
		staking.stakeSALT( saltAmountForEachUser );
		staking.transferStakedSaltFromAirdropToUser( msg.sender, saltAmountForEachUser );

    	_authorizedUsers.remove(msg.sender);
    	}

[G-02] `updateArbitrageIndicies()` from `PoolStats.sol` can be fully optimized

				uint64 poolIndex1 = _poolIndex( _weth, arbToken2, poolIDs );
				uint64 poolIndex2 = _poolIndex( arbToken2, arbToken3, poolIDs );
				uint64 poolIndex3 = _poolIndex( arbToken3, _weth, poolIDs );

				// Check if the indicies in storage have the correct values - and if not then update them
				ArbitrageIndicies memory indicies = _arbitrageIndicies[poolID];
				if ( ( poolIndex1 != indicies.index1 ) || ( poolIndex2 != indicies.index2 ) || ( poolIndex3 != indicies.index3 ) )

Solidity uses short-circuit to execute statements in conditions. This means, that for expression: A or B, when A evaluates to true - evaluation of B will be omitted (because true OR whatever is always true, thus there's no need to evaluate B).

This behavior means, that we don't need to calculate every _poolIndex() at once, but use this directly in the if statement. This will save us gas because: firstly, we won't be declaring redundant variables poolIndex1, poolIndex2, poolIndex3 - which are used only once - and secondly - because of the short-circuiting, when first expression returns true, we won't be performing another 2 _poolIndex() calls:

if ( ( _poolIndex( _weth, arbToken2, poolIDs ) != indicies.index1 ) || ( _poolIndex( arbToken2, arbToken3, poolIDs ) != indicies.index2 ) || ( _poolIndex( arbToken3, _weth, poolIDs ) != indicies.index3 ) )

[G-03] Use binary search in `_executeParameterChange()` in `Parameter.sol`

Since ParameterTypes is enum, its value represents consecutive integers. Instead of performing 26 comparisons (there are 1 if and 25 else-if conditional checks in _executeParameterChange()) - it might be a better idea to implement a binary-search in O(log n). The current time complexity is linear - O(n).

[G-04] `Proposals.sol` should be fully redesign by separating its functionality

The current implementation of Proposals.sol looks as follows. Every proposal: createConfirmationProposal(), proposeParameterBallot(), proposeTokenWhitelisting(), proposeTokenUnwhitelisting(), proposeSendSALT(), proposeCallContract(), proposeCountryInclusion(), proposeCountryExclusion(), proposeSetContractAddress(), proposeWebsiteUpdate() calls internal function _possiblyCreateProposal(). This function is like a prototype for creating proposal - it contains every parameter for every proposal type.

function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 )

however, proposals do not need all these parameters.

E.g. when we call proposeTokenWhitelisting(), we don't care about uint256 number1 - but we still need to pass it as 0: _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );.

When we call proposeCountryInclusion(), we don't care about address address1 and uint256 number1 - but we still need to pass it as address(0) and 0: _possiblyCreateProposal( ballotName, BallotType.EXCLUDE_COUNTRY, address(0), 0, country, description );.

This occurs for every proposal. In many proposals, we waste gas on passing redundant parameters (like 0, address(0)) to _possiblyCreateProposal(), which are not needed for the type of proposal we want.

Much better idea would be to save gas by coding each proposal directly. Even though we would spend more gas on the deployment (the code would be bigger) - we would save gas when executing contract (it wouldn't be needed to pass redundant parameters to internal function - actually, we won't need to call _possiblyCreateProposal() at all).

Our recommendation is to remove _possiblyCreateProposal() and implement each proposal creation directly in functions createConfirmationProposal(), proposeParameterBallot(), proposeTokenWhitelisting(), proposeTokenUnwhitelisting(), proposeSendSALT(), proposeCallContract(), proposeCountryInclusion(), proposeCountryExclusion(), proposeSetContractAddress(), proposeWebsiteUpdate(); instead of calling _possiblyCreateProposal() there.

[G-05] `tokenWhitelistingBallotWithTheMostVotes()` in `Proposals.sol` can be optimized

			if ( (yesTotal + noTotal) >= quorum ) // Make sure that quorum has been reached
			if ( yesTotal > noTotal )  // Make sure the token vote is favorable
			if ( yesTotal > mostYes )  // Make sure these are the most yes votes seen

In the list of ballots, it's very likely that there would be a lot of ballots which have more yeses than nos. Also, it's very likely that there would be a lot of ballots which reached the quorum. However, there will be only one ballot with the most yes votes. This implies, that we firstly should check yesTotal > mostYes.

If the current ballot has less yesTotal than mostYes, we do not care if it reached quorum or if the vote is favorable. Even if it is - it still not be the ballot with most yeses. This implies, that yesTotal > mostYes should be checked first.

Secondly, we don't care if quorum is reached, when there's more noTotal than yesTotal, thus yesTotal > noTotal should be evaluated before yesTotal + noTotal >= quorum.

To sum up, above code should be changed to:

if ( yesTotal > mostYes )  // Make sure these are the most yes votes seen
if ( yesTotal > noTotal )  // Make sure the token vote is favorable
if ( (yesTotal + noTotal) >= quorum ) // Make sure that quorum has been reached

[G-06] Every `change` function from `Config.sol` should be fully optimized

Every file from the below list:

./src/dao/DAOConfig.sol
./src/ExchangeConfig.sol
./src/pools/PoolsConfig.sol
./src/rewards/RewardsConfig.sol
./src/stable/StableConfig.sol
./src/staking/StakingConfig.sol

contains a public state variable which defines some protocol configuration. E.g., let's take a look at RewardsConfig.sol. It defines multiple of configurations:

uint256 public rewardsEmitterDailyPercentTimes1000 = 1000;
uint256 public emissionsWeeklyPercentTimes1000 = 500;
uint256 public stakingRewardsPercent = 50;
uint256 public percentRewardsSaltUSDS = 10;

Each of these configuration can be updated by calling a proper change* function. E.g., to update rewardsEmitterDailyPercentTimes1000 - we call changeRewardsEmitterDailyPercent(), to update emissionsWeeklyPercentTimes1000, we call changeEmissionsWeeklyPercent() and so on.

Each of these functions is implemented basically in the same way:

File: RewardsConfig.sol

	function changeEmissionsWeeklyPercent(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (emissionsWeeklyPercentTimes1000 < 1000)
                emissionsWeeklyPercentTimes1000 = emissionsWeeklyPercentTimes1000 + 250;
            }
        else
            {
            if (emissionsWeeklyPercentTimes1000 > 250)
                emissionsWeeklyPercentTimes1000 = emissionsWeeklyPercentTimes1000 - 250;
            }

		emit EmissionsWeeklyPercentChanged(emissionsWeeklyPercentTimes1000);
        }

It takes bool increase parameter which defines if we'll increase or decrease the state variable. Then it increase (of decrease) with previously defined adjustment. In the above example, we can increase/decrease emissionsWeeklyPercentTimes1000 by 250. Before each increase/decrease - function checks if the state variable will still be within previously defined range. E.g., for emissionsWeeklyPercentTimes1000 the maximum and minimum range is 1000 and 250. Let's examine some example to understand that: When we want to decrease emissionsWeeklyPercentTimes1000, it has to be greater than 250 (otherwise, decreasing it will make it not be within range). The same - when we want to increase it.

However, please notice that function reads the state variable multiple of times, instead of caching it and store in the local variable. When we want to increase/decrease emissionsWeeklyPercentTimes1000 variable - firstly we read its value in if statement. Secondly, we read its value to either increase/decrease it, then we read that value to assign the new value to it and finally, we read emissionsWeeklyPercentTimes1000 to emit EmissionsWeeklyPercentChanged() event.

Everything will use much less gas, when we will be able to cache the variable and move the event into if condition:

	function changeEmissionsWeeklyPercent(bool increase) external onlyOwner
        {
        uint256 emissionsWeeklyPercentTimes1000Cached = emissionsWeeklyPercentTimes1000;
        if (increase)
            {
            if (emissionsWeeklyPercentTimes1000Cached < 1000)
                emissionsWeeklyPercentTimes1000 = emissionsWeeklyPercentTimes1000Cached + 250;
                emit EmissionsWeeklyPercentChanged(emissionsWeeklyPercentTimes1000Cached + 250);
            }
        else
            {
            if (emissionsWeeklyPercentTimes1000Cached > 250)
                emissionsWeeklyPercentTimes1000 = emissionsWeeklyPercentTimes1000Cached - 250;
                emit EmissionsWeeklyPercentChanged(emissionsWeeklyPercentTimes1000Cached - 250);
            }

		
        }

This issue affects basically every change* function in every *Config.sol file. Every change* function is implemented like below:

	function changeSomething(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (stateVariable < RANGE_MAX)
                stateVariable = stateVariable + AMOUNT;
            }
        else
            {
            if (stateVariable > RANGE_MIN)
                stateVariable = stateVariable - AMOUNT;
            }

		emit Event(stateVariable);
        }

and can be rewritten to form:

	function changeSomething(bool increase) external onlyOwner
        {
        uint256 localVariableCache = stateVariable;
        if (increase)
            {
            if (localVariableCache < RANGE_MAX)
                stateVariable = localVariableCache + AMOUNT;
                emit Event(localVariableCache + AMOUNT);
            }
        else
            {
            if (localVariableCache > RANGE_MIN)
                stateVariable = localVariableCache - AMOUNT;
                emit Event(localVariableCache - AMOUNT);
            }

		emit Event(stateVariable);
        }

List of functions which should be optimized:

./src/dao/DAOConfig.sol:64:     function changeBootstrappingRewards(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:81:     function changePercentPolRewardsBurned(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:98:     function changeBaseBallotQuorumPercent(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:115:    function changeBallotDuration(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:132:    function changeRequiredProposalPercentStake(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:149:    function changeMaxPendingTokensForWhitelisting(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:166:    function changeArbitrageProfitsPercentPOL(bool increase) external onlyOwner
./src/dao/DAOConfig.sol:183:    function changeUpkeepRewardPercent(bool increase) external onlyOwner
./src/pools/PoolsConfig.sol:77: function changeMaximumWhitelistedPools(bool increase) external onlyOwner
./src/pools/PoolsConfig.sol:94: function changeMaximumInternalSwapPercentTimes1000(bool increase) external onlyOwner
./src/rewards/RewardsConfig.sol:36:     function changeRewardsEmitterDailyPercent(bool increase) external onlyOwner
./src/rewards/RewardsConfig.sol:52:     function changeEmissionsWeeklyPercent(bool increase) external onlyOwner
./src/rewards/RewardsConfig.sol:69:     function changeStakingRewardsPercent(bool increase) external onlyOwner
./src/rewards/RewardsConfig.sol:86:     function changePercentRewardsSaltUSDS(bool increase) external onlyOwner
./src/stable/StableConfig.sol:47:       function changeRewardPercentForCallingLiquidation(bool increase) external onlyOwner
./src/stable/StableConfig.sol:67:       function changeMaxRewardValueForCallingLiquidation(bool increase) external onlyOwner
./src/stable/StableConfig.sol:84:       function changeMinimumCollateralValueForBorrowing(bool increase) external onlyOwner
./src/stable/StableConfig.sol:101:      function changeInitialCollateralRatioPercent(bool increase) external onlyOwner
./src/stable/StableConfig.sol:118:      function changeMinimumCollateralRatioPercent(bool increase) external onlyOwner
./src/stable/StableConfig.sol:138:      function changePercentArbitrageProfitsForStablePOL(bool increase) external onlyOwner
./src/staking/StakingConfig.sol:34:     function changeMinUnstakeWeeks(bool increase) external onlyOwner
./src/staking/StakingConfig.sol:51:     function changeMaxUnstakeWeeks(bool increase) external onlyOwner
./src/staking/StakingConfig.sol:68:     function changeMinUnstakePercent(bool increase) external onlyOwner
./src/staking/StakingConfig.sol:85:     function changeModificationCooldown(bool increase) external onlyOwner

[G-07] Redundant calculations in `RewardsEmitter.sol`

File: RewardsEmitter.sol

115: 	uint256 sum = 0;
116:	for( uint256 i = 0; i < poolIDs.length; i++ )
[...]
128: 			sum += amountToAddForPool;

Function performUpkeep() intiializes variable sum and then, in every loop iteration - it adds the amountToAddForPool value to it. However, it seems that variable sum is nowhere used. Declaration of this variable (line 115) and adding amountToAddForPool to it (line 128) is redundant and can be removed. Function performUpkeep() does not utilize this variable at all, thus it can be simply removed.

[G-08] `excludedCountries` should be a different type

File: CollateralAndLiquidity.sol

mapping(string=>bool) public excludedCountries;

Since excludedCountries contains ISO 3166 Alpha-2 codes, which are 2-letters codes, the key type of this mapping should be bytes32 (or byte2). Moreover, it's more profitable in case of gas to operate on uint256 than bool, thus the value type of this mapping should be uint256

[G-09] Use binary search in `_executeApproval()` in `DAO.sol`

Since BallotType is enum, its value represents consecutive integers. Instead of performing 9 comparisons (there are 1 if and 8 else-if conditional checks in _executeApproval()) - it might be a better idea to implement a binary-search in O(log n). The current time complexity is linear - O(n).

[G-10] `require` statements which uses less gas should be called first

		require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
		require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
		require( amountRepaid > 0, "Cannot repay zero amount" );

Reading state variables (e.g. usdsBorrowedByUsers[msg.sender]) uses more gas than reading function parameter (e.g. amountRepaid). This implies, that amountRepaid > 0 costs less than amountRepaid <= usdsBorrowedByUsers[msg.sender]. Calling userShareForPool() costs even more. Above code should be rewritten to:

		require( amountRepaid > 0, "Cannot repay zero amount" );
		require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
		require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );

		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" );

Reading state variable collateralAndLiquidity and exchangeIsLive uses more gas than reading function's parameters tokenA, tokenB. This implies that order of require statements should be changed:

require( address(tokenA) != address(tokenB), "Cannot add liquidity for duplicate tokens" );
require( exchangeIsLive, "The exchange is not yet live" );
require( msg.sender == address(collateralAndLiquidity), "Pools.addLiquidity is only callable from the CollateralAndLiquidity contract" );

		require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
		require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

Reading state variable collateralAndLiquidity uses more gas than reading function's parameter liquidityToRemove, thus the order of require statements should be changed.

		require( poolsConfig.isWhitelisted( poolID ), "Invalid pool" );
		require( increaseShareAmount != 0, "Cannot increase zero share" );

Calling function isWhitelisted() uses more gas than reading function's parameter, thus the order of require statements should be changed.

[G-11] Change `>` to `>=` to avoid redundant `return`

		if ( user.virtualRewards > rewardsShare )
			return 0;

		return rewardsShare - user.virtualRewards;

When user.virtualRewards == rewardsShare, then the 2nd return will also return 0. Changing above code to:

		if ( user.virtualRewards >= rewardsShare )
			return 0;

		return rewardsShare - user.virtualRewards;

will save us a redundant substraction, when user.virtualRewards == rewardsShare (in that context, function will return with 0 ealier).

[G-12] It's possible to save one reading of `ballotFinalized` in `BootstrapBallot.sol` by changing its type

bool public ballotFinalized;
[...]
require( ! ballotFinalized, "Ballot has already been finalized" );
[...]
ballotFinalized = true;

ballotFinalized is used only in finalizeBallot() function. It's a state variable, thus reading it might be costly. If we change its type to uint256, we can save one reading by using ++ operator directly in require statement:

uint256 public ballotFinalized = 1;  // @audit: 1 means not finalized, 2 means finalized, we use 1;2 do avoid changing state from 0 to 1 in finalizeBallot()
[...]
require(ballotFinalized++ == 1, "Ballot has already been finalized" ); // @audit: on the second call, ballotFinalized will be 2, thus function will revert.
[...]
// @audit: this is no needed anymore: ballotFinalized = true;

[G-13] `step6()` in `Upkeep.sol` can be optimized

File: Upkeep.sol

	function step6() public onlySameContract
		{
		uint256 timeSinceLastUpkeep = block.timestamp - lastUpkeepTimeEmissions;
		emissions.performUpkeep(timeSinceLastUpkeep);

		lastUpkeepTimeEmissions = block.timestamp;
		}

There are two optimizations available.

Since lastUpkeepTimeEmissions is set to block.timestamp in the constructor() and later in step6(), we can be sure that lastUpkeepTimeEmissions <= block.timestamp, thus block.timestamp - lastUpkeepTimeEmissions can be unchecked.
There's no need to declare local timeSinceLastUpkeep variable, since it's used only once

	function step6() public onlySameContract
		{
		unchecked {
			emissions.performUpkeep(block.timestamp - lastUpkeepTimeEmissions);
			}
		lastUpkeepTimeEmissions = block.timestamp;
		}

[G-14] Use tautology to save one negation

Due to de Morgans Law: not A AND not B <=> not (A OR B). This means that below line of code:

require(address(pair.tokenA) != address(0) && address(pair.tokenB) != address(0), "This poolID does not exist");

can be changed to:

require(  !(address(pair.tokenA) == address(0) || address(pair.tokenB) == address(0)), "This poolID does not exist");

to save one negation.

File: ArbitrageSearch.sol

if ( (arbToken2 != _weth) && (arbToken3 != _weth) )

(arbToken2 != _weth) && (arbToken3 != _weth) is the same as ! ( (arbToken2 == _weth) || (arbToken3 == _weth) )

[G-15] Redundant safety check in `PoolMath.sol`

File: PoolMath.sol

 uint256 discriminant = B * B + 4 * A * C;

        // Compute the square root of the discriminant.
        uint256 sqrtDiscriminant = Math.sqrt(discriminant);

		// Safety check: make sure B is not greater than sqrtDiscriminant
		if ( B > sqrtDiscriminant )
			return 0;

Both A, B, C are declared as uint56, thus these variables are >= 0. We know that square root of B * B is equal to B. Thus square root of B * B + whatever has to be greater or equal (in case of rounding down) than B. This case is fulfilled here. Math.sqrt(B * B + 4 * A * C) >= B. This means that if ( B > sqrtDiscriminant ) will only be fulfilled when B == sqrtDiscriminant. Now let's check what will happen when B == sqrtDiscriminant. In line 203: swapAmount = ( sqrtDiscriminant - B ) / ( 2 * A ); we will have swapAmount = 0 / ( 2 * A ) = 0. This implies that function will return 0, the same as at line 199. This means, that lines 199-200 are redundant.

		// Safety check: make sure B is not greater than sqrtDiscriminant
		if ( B > sqrtDiscriminant )
			return 0;

Above safety check can be removed.

[G-16] Do not declare variables inside the loop

Variables should be declared outside the loop, so that they won't be re-declared on every loop iteration.

			for( uint256 i = 0; i < 8; i++ )
				{
				uint256 midpoint = (leftPoint + rightPoint) >> 1;

should be changed to:

			uint256 midpoint;
			for( uint256 i = 0; i < 8; i++ )
				{
				midpoint = (leftPoint + rightPoint) >> 1;

File: Proposal.sol

		for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
			{
			uint256 ballotID = _openBallotsForTokenWhitelisting.at(i);
			uint256 yesTotal = _votesCastForBallot[ballotID][Vote.YES];
			uint256 noTotal = _votesCastForBallot[ballotID][Vote.NO];

should be changed to:

			uint256 ballotID;
			uint256 yesTotal;
			uint256 noTotal;
		for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
			{
			ballotID = _openBallotsForTokenWhitelisting.at(i);
			yesTotal = _votesCastForBallot[ballotID][Vote.YES];
			noTotal = _votesCastForBallot[ballotID][Vote.NO];

The same issue occurs in:

PoolStats.sol - functions updateArbitrageIndicies() and _calculateArbitrageProfits()
RewardsEmitter.sol - functions addSALTRewards() and performUpkeep()
SaltRewards.sol - function _sendLiquidityRewards()
StakingRewards.sol - functions claimAllRewards(), addSALTRewards() and userCooldowns()
CollateralAndLiquidity.sol function findLiquidatableUsers()

[G-17] Increasing counters can be unchecked

File: AccessManager.sol

 geoVersion += 1;

Since geoVersion is being changed only at every DAO updates, it's almost impossible that this value would ever overflow - thus it can be unchecked.

File: PriceAggeregator.sol


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

There's no enough voters to these value ever overflow - thus they can be unchecked.

		if (price1 > 0)
			numNonZero++;

		if (price2 > 0)
			numNonZero++;

		if (price3 > 0)
			numNonZero++;

numNonZero++ can be unchecked.

File: Staking.sol

unstakeID = nextUnstakeID++;

This won't overflow, thus can be unchecked.

[G-18] There's no need to declare variables used only once

File: AccessManager.sol

		bytes32 messageHash = keccak256(abi.encodePacked(block.chainid, geoVersion, wallet));

		return SigningTools._verifySignature(messageHash, signature);

messageHash is used only once, thus there's no need to declare it at all:

return SigningTools._verifySignature(keccak256(abi.encodePacked(block.chainid, geoVersion, wallet)), signature);

			uint256 saltBalance = exchangeConfig.salt().balanceOf( address(this) );
			require( saltBalance >= bootstrappingRewards * 2, "Whitelisting is not currently possible due to insufficient bootstrapping rewards" );

			// Fail to whitelist for now if this isn't the whitelisting proposal with the most votes - can try again later.
			uint256 bestWhitelistingBallotID = proposals.tokenWhitelistingBallotWithTheMostVotes();
			require( bestWhitelistingBallotID == ballotID, "Only the token whitelisting ballot with the most votes can be finalized" );

[...]
			bytes32 pool1 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.wbtc() );
			bytes32 pool2 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.weth() );

			// Send the initial bootstrappingRewards to promote initial liquidity on these two newly whitelisted pools
			AddedReward[] memory addedRewards = new AddedReward[](2);
			addedRewards[0] = AddedReward( pool1, bootstrappingRewards );
			addedRewards[1] = AddedReward( pool2, bootstrappingRewards );

Variables saltBalance, bestWhitelistingBallotID, pool1, pool2 are used only once, thus there's no need to declare them at all:

			require( exchangeConfig.salt().balanceOf( address(this) ) >= bootstrappingRewards * 2, "Whitelisting is not currently possible due to insufficient bootstrapping rewards" );

			// Fail to whitelist for now if this isn't the whitelisting proposal with the most votes - can try again later.
			require( proposals.tokenWhitelistingBallotWithTheMostVotes() == ballotID, "Only the token whitelisting ballot with the most votes can be finalized" );

[...]

			// Send the initial bootstrappingRewards to promote initial liquidity on these two newly whitelisted pools
			AddedReward[] memory addedRewards = new AddedReward[](2);
			addedRewards[0] = AddedReward( PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.wbtc() ), bootstrappingRewards );
			addedRewards[1] = AddedReward( PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.weth() ), bootstrappingRewards );

		bytes32 poolID1 = PoolUtils._poolID( token, wbtc );
		if ( isWhitelisted(poolID1) )
			return true;

		bytes32 poolID2 = PoolUtils._poolID( token, weth );
		if ( isWhitelisted(poolID2) )

Variables poolID1 and poolID2 are used only once, thus there no need to declare them at all:

		if ( isWhitelisted(PoolUtils._poolID( token, wbtc )) )
			return true;

		if ( isWhitelisted(PoolUtils._poolID( token, weth )) )

[G-19] `require` statemets which uses less gas should be executed first

Whenever we use 2 require statements, we assume that both of them won't fail. If the first one will fail (thus function will revert), the second one won't be executed. Based on that, it's better to execute the require statement which uses less gas first. If it would revert, user wouldn't be paying gas for the 2nd (more expensive) revert.

File: ManagedWallet.sol

		require( msg.sender == mainWallet, "Only the current mainWallet can propose changes" );
		require( _proposedMainWallet != address(0), "_proposedMainWallet cannot be the zero address" );
		require( _proposedConfirmationWallet != address(0), "_proposedConfirmationWallet cannot be the zero address" );

Reading state variable costs more than reading a function parameter. This means, that above code should be changed to:

		require( _proposedMainWallet != address(0), "_proposedMainWallet cannot be the zero address" );
		require( _proposedConfirmationWallet != address(0), "_proposedConfirmationWallet cannot be the zero address" );
		require( msg.sender == mainWallet, "Only the current mainWallet can propose changes" );

The same issue occurs in other instances:

File: USDS.sol

		require( msg.sender == address(collateralAndLiquidity), "USDS.mintTo is only callable from the Collateral contract" );
		require( amount > 0, "Cannot mint zero USDS" );

Reading stare variable address(collateralAndLiquidity) costs more than function's parameter amount, thus order of the require statements should be changed.

[G-20] Calculations which won't underflow should be unchecked

		uint256 amountToSendToTeam = claimedSALT / 10;
		salt.safeTransfer( exchangeConfig.managedTeamWallet().mainWallet(), amountToSendToTeam );
		emit TeamRewardsTransferred(amountToSendToTeam);

		uint256 remainingSALT = claimedSALT - amountToSendToTeam;

Since amountToSendToTeam = claimedSALT / 10, we know that claimedSALT >= amountToSendToTeam, thus line uint256 remainingSALT = claimedSALT - amountToSendToTeam can be unchecked.

File: CollateralAndLiquidity.sol

    	require( _userDeposits[msg.sender][token] >= amount, "Insufficient balance to withdraw specified amount" );
        require( amount > PoolUtils.DUST, "Withdraw amount too small");

		_userDeposits[msg.sender][token] -= amount;

Because _userDeposits[msg.sender][token] >= amount, _userDeposits[msg.sender][token] -= amount won't underflow and can be unchecked.

File: SaltRewards.sol

uint256 stakingRewardsAmount = ( remainingRewards * rewardsConfig.stakingRewardsPercent() ) / 100;
uint256 liquidityRewardsAmount = remainingRewards - stakingRewardsAmount;

rewardsConfig.stakingRewardsPercent() is defined as value in range [25, 75]. This means that remainingRewards * rewardsConfig.stakingRewardsPercent() ) / 100 will always be lower or equal to remainingRewards. According to that remainingRewards - stakingRewardsAmount won't underflow, thus can be unchecked.

		require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
		require( amountRepaid > 0, "Cannot repay zero amount" );

		// Decrease the borrowed amount for the user
		usdsBorrowedByUsers[msg.sender] -= amountRepaid;

Because amountRepaid <= usdsBorrowedByUsers[msg.sender], usdsBorrowedByUsers[msg.sender] -= amountRepaid won't underflow and can be unchecked.

if ( block.timestamp >= cooldownExpiration )
				cooldowns[i] = 0;
			else
				cooldowns[i] = cooldownExpiration - block.timestamp;
			}

Because block.timestamp < cooldownExpiration, cooldownExpiration - block.timestamp won't underflow and can be unchecked.

[G-21] Do not perform `safeTransfer` when amount is 0

0-transfers does not change anything and are considered as a waste of gas. Before performing a transfer - make sure that the transferred amount is greater than 0.

		(uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block.timestamp );

		// Send the withdrawn tokens to the Liquidizer so that the tokens can be swapped to USDS and burned as needed.
		tokenA.safeTransfer( address(liquidizer), reclaimedA );
		tokenB.safeTransfer( address(liquidizer), reclaimedB );

Add additional check:

if (reclaimedA > 0) tokenA.safeTransfer( address(liquidizer), reclaimedA );;
if (reclaimedB > 0) tokenB.safeTransfer( address(liquidizer), reclaimedB );

		tokenA.safeTransferFrom(msg.sender, address(this), addedAmountA );
		tokenB.safeTransferFrom(msg.sender, address(this), addedAmountB );

Add additional check which guarantees that addedAmountA and addedAmountB are greater than 0:

if (addedAmountA > 0) tokenA.safeTransferFrom(msg.sender, address(this), addedAmountA );
if (addedAmountB > 0) tokenB.safeTransferFrom(msg.sender, address(this), addedAmountB );

[G-22] Use `delete` instead of assigning mapping variable to `0`

_arbitrageProfits[ poolIDs[i] ] = 0;

File: ArbitrageSearch.sol

cooldowns[i] = 0;

[G-23] Loops which iterates only a few times can be coded directly

for( uint256 i = 0; i < 8; i++ )

The loop executes only 7 times and is very short. It might be reasonable to use the code inside the loop directly, to save gas from the loop initialization and checking i < 8 conditions on every loop iteration.

[G-24] Cache state variables when reading them more than once