Salty.IO - 0xRobocop's results

Lines of code

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

Vulnerability details

Impact

Staking in SALTY pools happens automatically when adding liquidity. In order to track the accrued rewards, the code "simulates" the amount of virtual rewards that need to be added given the increase of shares and lend this amount to the user. So, when computing the real rewards for a given user, the code will compute its rewards based on the totalRewards of the given pool minus the virtual rewards. The following code computes the virtual rewards for a user:

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

uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

Basically, it aims to maintain the current ratio of totalRewards and existingTotalShares. The issue with this is that allows the first depositor to set the ratio too high by donating some SALT tokens to the contract. For example, consider the following values:

uint256 virtualRewardsToAdd = Math.ceilDiv( 1000e18 * 200e18, 202 );

The returned value is in order of 39-40 digits. Which is beyond what 128 bits can represent:

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

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

This will broke the reward computations. For a more concrete example look the PoC.

Proof of Concept

The following coded PoC showcase an scenario where the first depositor set the rewards / shares ratio too high, causing the rewards system to get broken. Specifically, it shows how the sum of the claimable rewards for each user is greater than the SALT balance of the contract.

It should be pasted under Staking/tests/StakingRewards.t.sol.

function testUserCanBrickRewards() public {
        vm.startPrank(DEPLOYER);
        // Alice is the first depositor to poolIDs[1] and she deposited the minimum amounts 101 and 101 of both tokens.
        // Hence, alice will get 202 shares.          
        stakingRewards.externalIncreaseUserShare(alice, poolIDs[1], 202, true);
        assertEq(stakingRewards.userShareForPool(alice, poolIDs[1]), 202);
        vm.stopPrank();
        
        // Alice adds 100 SALT as rewards to the pool.
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(poolIDs[1], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        
        // Bob deposits 100 DAI and 100 USDS he will receive (202 * 100e8) / 101 = 200e18 shares.
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(bob, poolIDs[1], 200e18, true);
        assertEq(stakingRewards.userShareForPool(bob, poolIDs[1]), 200e18);
        vm.stopPrank();

        // Charlie deposits 10000 DAI and 10000 USDS he will receive (202 * 10000e8) / 101 = 20000e18 shares.
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(charlie, poolIDs[1], 20000e18, true);
        assertEq(stakingRewards.userShareForPool(charlie, poolIDs[1]), 20000e18);
        vm.stopPrank();

        // Observe how virtual rewards are broken. 
        uint256 virtualRewardsAlice = stakingRewards.userVirtualRewardsForPool(alice, poolIDs[1]);
        uint256 virtualRewardsBob = stakingRewards.userVirtualRewardsForPool(bob, poolIDs[1]);
        uint256 virtualRewardsCharlie = stakingRewards.userVirtualRewardsForPool(charlie, poolIDs[1]);

        console.log("Alice virtual rewards %s", virtualRewardsAlice);
        console.log("Bob virtual rewards %s", virtualRewardsBob);
        console.log("Charlie virtual rewards %s", virtualRewardsCharlie);

        // Observe the amount of claimable rewards.
        uint256 aliceRewardAfter = stakingRewards.userRewardForPool(alice, poolIDs[1]);
        uint256 bobRewardAfter = stakingRewards.userRewardForPool(bob, poolIDs[1]);
        uint256 charlieRewardAfter = stakingRewards.userRewardForPool(charlie, poolIDs[1]);

        console.log("Alice rewards %s", aliceRewardAfter);
        console.log("Bob rewards %s", bobRewardAfter);
        console.log("Charlie rewards %s", charlieRewardAfter);

        // The sum of claimable rewards is greater than 1000e18 SALT.
        uint256 sumOfRewards = aliceRewardAfter + bobRewardAfter + charlieRewardAfter;  
        console.log("All rewards &s", sumOfRewards);

        bytes32[] memory poolIDs2;

        poolIDs2 = new bytes32[](1);

        poolIDs2[0] = poolIDs[1];

        // It reverts
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        vm.startPrank(charlie);
        stakingRewards.claimAllRewards(poolIDs2);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Some options:

• Make the function addRewards in the StakingRewards contract permissioned. In this way, all rewards will need to go through the emitter first.

• Do not let the first depositor to manipulate the initial ratio of rewards / share. It is possible for every pool to burn the initial 10000 shares and starts with an initial small amount of rewards, kind of simulating being the first depositor.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Any user can avoid liquidation by incrementing its cooldownExpiration.

During liquidations the logic of the liquidateUser function will call _decreaseUserShare for the liquidated user. The issue is that it sets the flag for the cooldown check to true:

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

If the flag is set to true, then the code will validate that block.timestamp > cooldownExperation.

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

Because of this, the user could add the minimum amount of liquidity to the WETH / WBTC pool, incrementing his coolDownExpiration and avoid any liquidation even if its position is still unhealthy.

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

Proof of Concept

A coded PoC is provided, paste it under the tests/CollateralAndLiquidity.t.sol file:

function testUserCanAvoidLiquidation() public {
     // Alice deposits 100 WBTC and 100 WETH and borrows the maximum usds possible.
     vm.startPrank( alice );
     collateralAndLiquidity.depositCollateralAndIncreaseShare(100e8, 100e18, 0, block.timestamp, false );
     uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
     assertFalse(maxUSDS == 0);
     collateralAndLiquidity.borrowUSDS( maxUSDS );
     vm.stopPrank();

     // Alice cannot be liquidated
     assertFalse(collateralAndLiquidity.canUserBeLiquidated(alice));

     vm.warp( block.timestamp + 1 days );

     // Alice adds more collateral before the crash of the price.
     // @audit With this it resets its cooldown period.
     vm.startPrank( alice );
     collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101e10, 0, block.timestamp, false);
     vm.stopPrank();

     // Artificially crash the collateral price
     _crashCollateralPrice();

     // Make bob to do a deposit.
     vm.startPrank( bob );
     collateralAndLiquidity.depositCollateralAndIncreaseShare(100e8, 100e18, 0, block.timestamp, false );
     vm.stopPrank();

     // Alice account is liquiditable
     assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

     // Liquidation does not go through due to cooldown period.
     vm.expectRevert( "Must wait for the cooldown to expire" );
     vm.prank(bob);
     collateralAndLiquidity.liquidateUser(alice);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Set the cooldown flag to false instead of true.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Malicious users can prevent the DAO from accruing POL (salt/usds and dai/usds) and to lose the SALT withdrawn tokens from POL.

Background

When a liquidation happens, the CollateralAndLiquidity contract sends the recently liquidated WETH and WBTC to the Liquidizer contract and also makes it to increment it's usdsThatShouldBeBurned value by an amount equal to the recently liquidated debt.

The idea is that the Liquidizer contract will sell DAI, WETH and WBTC for USDS and then it will burn an amount equal to usdsThatShouldBeBurned. Basically, this is the mechanism to which the system takes out of circulation the liquidated debt's USDS.

An important note is that when the Liquidizer has less USDS balance than the value of usdsThatShouldBeBurned it will burn what it have and then will call dao.withdrawPOL for USDS / SALT and DAI / USDS. The SALT withdrawn will get lost (design decision) to no put sell pressure on USDS.

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

Proof of Concept

Exploit

The security vulnerability emerges due to a logic error in the repayUSDS function. This flaw allows any user to exert pressure on the Liquidizer contract, leading to constant POL withdrawals. In the mentioned function we can see the following:

// @audit It increments the amount of USDS to burn in Liquidizer but the transfer
//        of USDS goes to the USDS contract.
usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
liquidizer.incrementBurnableUSDS( amountRepaid );

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

We can see that the logic makes Liquidizer to increment the usdsThatShouldBeBurned value, but its USDS balance wont increment. Malicious users can exacerbate the problem by making loops of borrowing, which is free, and repayments.

Tools Used

Manual Review

Recommended Mitigation Steps

Two options:

• Instead of transferring the USDS to the USDS contract, they should be transferred to the liquidizer contract.

• Do not call liquidizer.incrementBurnableUSDS.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

User may not be able to create another proposal for a long time even if its token whitelisting proposal is accepted by the community.

Users can propose a token whitelisting ballot via the proposeTokenWhitelisting function. Where the code makes the following validation:

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

require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );

The issue is that when a token is whitelisted, the poolsConfig contract actually whitelist two pools, one with weth and the other with wbtc. Hence, when the whitelisted pool array is at its maximum capacity minus one, the proposal wont go through even if its the most voted on the array of pending whitelistings, due to the validation on poolsConfig:

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

This will cause the creator an indefinite DoS because its proposal cannot be finalized unless people change their votes.

Proof of Concept

The maximum number of whitelisted pools is 100. The number of initial whitelisted pools is 9:

• SALT-WBTC
• SALT-WETH
• USDS-SALT
• USDS-DAI
• USDS-WETH
• USDS-WBTC
• DAI-WETH
• DAI-WBTC
• WETH-WBTC

It is possible to reach the state where there are 99 whitelisted pools. And the proposeTokenWhitelisting function will allow to create another whitelisting proposal.

Tools Used

Manual Review

Recommended Mitigation Steps

The correct validation should be:

require( poolsConfig.numberOfWhitelistedPools() + 1 < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When liquidating a user, the logic of the function liquidateUser removes all the liquidity of the liquidated account.

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

The issue arises when the user been liquidated is also the last liquidity provider in the pool. This is because when removing liquidity there is a requirement for a minimum amount of reserves. Hence, the liquidation will not go through.

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

Proof of Concept

A coded PoC is provided, paste it under the tests/CollateralAndLiquidity.t.sol file:

function testLastLiquidityProviderCannotBeLiquidiated() public {
        // Alice deposits 100 WBTC and 100 WETH and borrows the maximum usds possible.
        vm.startPrank( alice );
        collateralAndLiquidity.depositCollateralAndIncreaseShare(100e8, 100e18, 0, block.timestamp, false );
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        assertFalse(maxUSDS == 0);
        collateralAndLiquidity.borrowUSDS( maxUSDS );
        vm.stopPrank();

        // Alice cannot be liquidated
        assertFalse(collateralAndLiquidity.canUserBeLiquidated(alice));
 
        vm.warp( block.timestamp + 1 days );

        // Artificially crash the collateral price
        _crashCollateralPrice();

        // Alice account is liquiditable
        assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

        // Liquidation does not go through due reserves limitation.
        vm.expectRevert( "Insufficient reserves after liquidity removal" );
        vm.prank(bob);
        collateralAndLiquidity.liquidateUser(alice);
}

Tools Used

Manual Review

Recommended Mitigation Steps

In the event of a liquidation allow it go through even if the reserves become less than dust.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The ManagedWallet contract has 4 accounts mainWallet, confirmationWallet, proposedMainWallet and proposedConfirmationWallet. The mainWallet is used in the Salty protocol as the recipient of the accrued rewards for the team.

The mainWallet can select another address to be the new mainWallet. But, to avoid any mistake on this change, the contract implements a two-step transfership of this role. For this purpose the confirmationWallet is used.

The issue is that confirmationWallet is forced to either accept the new proposedMainWallet or the contract will stay in the same state without having the opportunity to change the mainWallet.

Proof of Concept

Once the mainWallet has proposed a proposedMainWallet then it cannot change it:

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

So, in case an incorrect account is proposed as the new main wallet, the confirmation wallet will reject it, but given that the mainWallet cannot change the proposed wallet, then the contract will never change its state and the business logic of been able to change the mainWallet will get broken.

Tools Used

Manual Review

Recommended Mitigation Steps

After some time, the mainWallet should be able to change proposedMainWallet.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The pool contract works on the assumption that the reserves are greater than a declared DUST amount. This is enforced in every operation involving the movement of liquidity reserves.

Notably, in the removeLiquidity function, the logic validates that the final amount of reserves is not below the declared dust amounts. The issue is that it does that incorrectly, see:

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

It just validates that reserve0 is greater than DUST, but not for reserves1.

Proof of Concept

Link to the incorrect validation:

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

Tools Used

Manual Review

Recommended Mitigation Steps

Change one of the reserves0 to reserves1.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The quorum needed for proposals to go through is computed based on a percentage of the current xSALT staked:

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

Given that this value is dynamic, in other words, it depends on the current amount of xSALT staked at the moment when the ballot wants to be finalized. Then, it allows users to game the accounting of the quorum by casting their vote and then unstaking.

Proof of Concept

To make the example clear, consider the following scenario:

• totalxSALT = 500
• percentageQuorum = 25%
• quorumNeeded = 0.25 * 500 = 125 xSALT

Then, a given user has 100 xSALT balance. The user can cast his vote and then unstake, then:

• totalxSALT = 400
• percentageQuorum = 25%
• quorumNeeded = 400 * 0.25 = 100 xSALT

This new state allows the user to reach the quorum for its proposal. Which is unfair because the casted votes are still counting as amount of votes but they do not contribute to the computed final quorum.

Tools Used

Manual Review

Recommended Mitigation Steps

During the creation of a ballot the needed quorum should be saved within the ballot information. When someone wants to finalize the ballot, the saved value can be compared with the current needed quorum and the code can choose the greatest of them. For example, in the above example the next would have happened (pseduo-solidity):

uint256 currentQuorum = requiredQuorumForBallotType(ballot);

uint256 neededQuorum = max(currentQuourum, savedQuorum);

// neededQuorum will be 125 instead of 100

It is also important to consider a mechanism to finalize ballots after a period of time even if they did not reach the quorum.

Assessed type

Governance

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

L-03 Some DAO proposals can be hard to propose. Some proposals such as proposeCountryInclusion, proposeCountryExclusion, proposeSetContractAddress and proposeWebsiteUpdate can only be created once. For example, in other words, proposing a ballot for changing the address for priceFeed_1 is only possible if there are not active ballots for the same purpose.

So, this allows a griefing attack vector where a well-funded user may try to always spam the creation of these proposals with a dummy address after the finalization of the previous one. This can be a challenge for governance participants, since they first need to coordinate efforts to reach the quorum in order to finalize the active dummy proposal and then ensure that they can propose the valid one previous in the block than the attacker.

Recommendation:

Information can included for the identification of the ballot in order to discriminate them even if they are the same type of proposal. For example, in the case of the price feeds, the proposed new address can be used to differentiate between the attacker one and the valid one.

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

L-04 Reaching quorum to finalize a ballot is a too hard requirement. The current logic on making proposals only allow a user to have at most one active proposal:

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

The issue is that in order to finalize a proposal even if it was not approved it need to reach a certain quorum of votes:

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

This is a too hard requirement, since it makes the user dependent on the participation that its proposal can get regardless if the direction is positive or negative. Some proposals encourage the community to vote (because they can only be proposed once, like change a parameter), but other ones such as proposeCallContract do not.

Users can still unstake and migrate their tokens to another account, but this will take almost a year in order to unstake completely.

Recommendation:

To avoid a DoS state on the proposals made by users due to a small activity of voters, it should be possible to finalize non-reached quorum ballots after some period of time.

Lines of code

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

Vulnerability details

Impact

The rewards earned from the DAOs POL are distributed among the team wallet, then part of the remaining rewards are burned and the rest are kept as DAOs balance.

The issue, is that is possible for the DAO to claim SALT rewards without sending the team's share to the team wallet and without burning the amount that should be burned.

Proof of Concept

The first step during the upkeep functionality is to perform the upkeep on the liquidizer contract.

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

During that, if the amount of USDS to be burned is greater than the current balance of the liquidizer contract, then the code will withdraw some POL from the usds/dai and usds/salt pools:

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

When removing liquidity, the code will decrease the dao's share and in doing so it will send to it some rewards proportional to the amount of shares decreased:

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

It is important note that pool rewards do not necessary comes from the emitter, but they can be added by third party protocols via the addRewards function in the StakingRewards contract:

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

Tools Used

Manual Review

Recommended Mitigation Steps

Two options:

• Save the amount of rewards received when withdrawing some POL, so they can be distributed and burned.

• Make the call to claim all the rewards at the beginning of upkeep.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The DAO contract cannot handle any token beside SALT tokens. So, if tokens like USDS or DAI were in its balance, they will be lost forever.

This can happen during the upkeep calls. Basically, during upkeep the contract takes some percentage from the arbitrage profits and use them to form POL for the DAO (usds/dai and salt/usds). The DAO swaps the ETH for both of the needed tokens and then adds the liquidity using the zapping flag to true.

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

Zapping will compute the amount of either tokenA or tokenB to swap in order to add liquidity at the final ratio of reserves after the swap. But, it is important to note that the zap computations do no take into account that the same pool may get arbitraged atomically, changing the ratio of reserves a little.

As a consequence, some of the USDS and DAI tokens will be send back to the DAO contract:

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L110C3-L115C1

Proof of Concept

The following coded PoC should be pasted into root_tests/Upkeep.t.sol, actually its the same test that can be found at the end of the file with some added lines to showcase the issue. Specifically, it shows how the USDS and DAI balance of the DAO are zero before upkeep and how both are greater than zero after upkeep.

function testDoublePerformUpkeep() public {

        _setupLiquidity();
        _generateArbitrageProfits(false);

    	// Dummy WBTC and WETH to send to Liquidizer
    	vm.prank(DEPLOYER);
    	weth.transfer( address(liquidizer), 50 ether );

    	// Indicate that some USDS should be burned
    	vm.prank( address(collateralAndLiquidity));
    	liquidizer.incrementBurnableUSDS( 40 ether);

    	// Mimic arbitrage profits deposited as WETH for the DAO
    	vm.prank(DEPLOYER);
    	weth.transfer(address(dao), 100 ether);

    	vm.startPrank(address(dao));
    	weth.approve(address(pools), 100 ether);
    	pools.deposit(weth, 100 ether);
    	vm.stopPrank();

        // === Perform upkeep ===
        address upkeepCaller = address(0x9999);

        uint256 daiDaoBalanceBefore = dai.balanceOf(address(dao));
        uint256 usdsDaoBalanceBefore = usds.balanceOf(address(dao));

        assertEq(daiDaoBalanceBefore, 0);
        assertEq(usdsDaoBalanceBefore, 0);

        vm.prank(upkeepCaller);
        upkeep.performUpkeep();
        // ==================

        _secondPerformUpkeep();

        uint256 daiDaoBalanceAfter = dai.balanceOf(address(dao));
        uint256 usdsDaoBalanceAfter = usds.balanceOf(address(dao));

        assertTrue(daiDaoBalanceAfter > 0);
        assertTrue(usdsDaoBalanceAfter > 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

The leftovers of USDS or DAI should be send to liquidizer so they can be handled.

Assessed type

Other

L-01 Bootstrap ballot can get bricked.

When finalizeBallot is called and the majority voted yes, then the code initiates the distribution of SALT tokens calling the distributionApproved function of the InitialDistribution contract which should have been transferred the 100M SALT tokens previously.

The issue arises because users can still cast their vote even after the ballot completion time has passed. So it is possible for users to vote negative and outnumber the "yes" votes:

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

If SALT supply were sent non-atomically with the call to finalizeBallot it will cause the SALT tokens to get stucked in the InitialDistribution contract:

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

Recommendations:

• Do not allow to vote if block.timestamp > completion.
• Or make the transfer of SALT tokens and the call to finalizeBallot atomically.

L-02 Not claimed SALT from airdrop will get stucked.

The initial distribution of SALT tokens take into account an initial airdrop where whitelisted users that casted their votes can claim their SALT rewards. The issue is that in case some users do not claim these SALT tokens, there is no other way to take this tokens out of the contract.

Recommendation: Send the remaining SALT balance from the airdrop contract to the DAO after a specified deadline.

L-03 Some DAO proposals can be hard to propose.

Some proposals such as proposeCountryInclusion, proposeCountryExclusion, proposeSetContractAddress and proposeWebsiteUpdate can only be created once. For example, in other words, proposing a ballot for changing the address for priceFeed_1 is only possible if there are not active ballots for the same purpose.

So, this allows a griefing attack vector where a well-funded user may try to always spam the creation of these proposals with a dummy address after the finalization of the previous one. This can be a challenge for governance participants, since they first need to coordinate efforts to reach the quorum in order to finalize the active dummy proposal and then ensure that they can propose the valid one previous in the block than the attacker.

Recommendation:

• Information can included for the identification of the ballot in order to discriminate them even if they are the same type of proposal. For example, in the case of the price feeds, the proposed new address can be used to differentiate between the attacker one and the valid one.

L-04 Reaching quorum to finalize a ballot is a too hard requirement.

The current logic on making proposals only allow a user to have at most one active proposal:

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

The issue is that in order to finalize a proposal even if it was not approved it need to reach a certain quorum of votes:

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

This is a too hard requirement, since it makes the user dependent on the participation that its proposal can get regardless if the direction is positive or negative. Some proposals encourage the community to vote (because they can only be proposed once, like change a parameter), but other ones such as proposeCallContract do not.

Users can still unstake and migrate their tokens to another account, but this will take almost a year in order to unstake completely.

Recommendation:

• To avoid a DoS state on the proposals made by users due to a small activity of voters, it should be possible to finalize non-reached quorum ballots after some period of time.