Salty.IO - juancito's results

Lines of code

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

Vulnerability details

Impact

Users can avoid liquidations by abusing the cooldown mechanism

Vulnerability Details

The problem is that when a liquidation is attempted, it checks for the cooldown as seen here (last parameter true):

// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
_decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);

CollateralAndLiquidity.sol#L153-L154

This means that if the user has an active cooldown, it will make the transaction revert:

if ( useCooldown ) {
    require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

StakingRewards.sol#L104-L107

A cooldown can be easily triggered by a user, just by depositing some collateral. This is done via _increaseUserShare(), which sets the cooldown.

The minimum amount of collateral is determined while adding liquidity, which can be as low as the DUST amount, which is 100 wei of a token.

So, the user will avoid liquidation during the whole duration of the cooldown. This can be performed repeatedly by the user, after the cooldown expires, at a minimum cost of 100 wei of tokens each time.

A POC is provided to assess the validity of the claimings.

Proof of Concept

1. Add this test to src/stable/tests/CollateralAndLiquidity.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://sepolia.gateway.tenderly.co --mt "testAvoidLiquidation"

function testAvoidLiquidation() public {
    // IMPORTANT NOTE
    // This is an exact copy of the `testLiquidatePosition` test up to the attacked marked below as <<ATTACK>>

    assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0);

    assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
    assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
    assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

    // Total needs to be worth at least $2500
    uint256 depositedWBTC = (1000 ether * 10 ** 8) / priceAggregator.getPriceBTC();
    uint256 depositedWETH = (1000 ether * 10 ** 18) / priceAggregator.getPriceETH();

    (uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
    assertEq(reserveWBTC, 0, "reserveWBTC doesn't start as zero");
    assertEq(reserveWETH, 0, "reserveWETH doesn't start as zero");

    // Alice will deposit collateral and borrow max USDS
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        depositedWBTC, depositedWETH, 0, block.timestamp, false
    );

    uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    assertEq(maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS");

    // Deposit again
    vm.warp(block.timestamp + 1 hours);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        depositedWBTC, depositedWETH, 0, block.timestamp, false
    );

    // Account for both deposits
    depositedWBTC = depositedWBTC * 2;
    depositedWETH = depositedWETH * 2;
    vm.stopPrank();

    // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
    vm.prank(DEPLOYER);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(1 * 10 ** 8, 1 ether, 0, block.timestamp, false);

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

    vm.startPrank(alice);
    maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

    vm.startPrank(alice);
    vm.warp(block.timestamp + 1 hours);

    maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS(maxUSDS);
    vm.stopPrank();

    assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1);

    uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
    assertEq(maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral");

    {
        uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);

        uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
        assertEq(
            collateralAndLiquidity.usdsBorrowedByUsers(alice),
            aliceBorrowedUSDS,
            "Alice amount USDS borrowed not what she has"
        );

        // Borrowed USDS should be about 50% of the aliceCollateralValue
        assertTrue(aliceBorrowedUSDS > (aliceCollateralValue * 499 / 1000), "Alice did not borrow sufficient USDS");
        assertTrue(aliceBorrowedUSDS < (aliceCollateralValue * 501 / 1000), "Alice did not borrow sufficient USDS");
    }

    // Try and fail to liquidate alice
    vm.expectRevert("User cannot be liquidated");
    vm.prank(bob);
    collateralAndLiquidity.liquidateUser(alice);

    // Artificially crash the collateral price
    _crashCollateralPrice();

    // Delay before the liquidation
    vm.warp(block.timestamp + 1 days);


    // <<ATTACK>>

    // Provide Alice with some tokens
    uint256 dust = PoolUtils.DUST + 1;
    assertEq(dust, 101);
    deal(address(wbtc), alice, dust);
    deal(address(weth), alice, dust);

    // Alice knows her position can be liquidated
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

    // Alice performs the attack. She deposits a minimum amount of tokens
    // This way she prevents being liquidated for the whole duration of the cooldown
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        dust, dust, 0, block.timestamp, false
    );

    // Alice's position is still liquidatable
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

    // Alice's position can't be liquidated due to the cooldown
    vm.prank(bob);
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
}

Recommended Mitigation Steps

Do not use cooldown when liquidating users:

    // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
-   _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);
+   _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, false);

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Protocol Owned Liquidity from the DAO is drained as the Liquidizer doesn't have enough USDS to cover the upkeep process.

Proof of Concept

The problem is that USDS tokens are sent to the usds contract instead of the liquidizer.

// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

CollateralAndLiquidity.sol#L124-L125

First of all note how the comment refers to a USDS.performUpkeep function, which doesn't exist on the USDS contract, but should refer to the upkeep in the Liquidizer contract.

The correct implementation can be seen when positions are liquidated, as they transfer assets to the Liquidizer, as well as incrementing the burnable USDS.

In contrast, the repayUSDS() function increases the burnable USDS without transfering the asset to the liquidizer (as it is transfered to the USDS contract instead):

    // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
    usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

    // Have USDS remember that the USDS should be burned
👉  liquidizer.incrementBurnableUSDS( amountRepaid );

CollateralAndLiquidity.sol#L124-L128

When incrementBurnableUSDS() is called, it increases the usdsThatShouldBeBurned variable.

This is used during the liquidizer upkeep, but as the incrementBurnableUSDS will be increased without actually sending USDS this check will return false, leading to the following action:

    // The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
    _burnUSDS( usdsBalance );
    usdsThatShouldBeBurned -= usdsBalance;

👉  // As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
👉  // send the underlying tokens here to be swapped to USDS
    dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
    dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);

Liquidizer.sol#L117-L124

In short, it will "withdraw Protocol Owned Liquidity from the DAO" as described here to cover the shortage of USDS in the Liquidizer contract, slowly draining the POL of the DAO.

Recommended Mitigation Steps

Send the assets to the liquidizer:

-   // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
-   usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
+   // Have the user send the USDS to the Liquidizer contract so that it can later be burned (on Liquidizer.performUpkeep)
+   usds.safeTransferFrom(msg.sender, address(liquidizer), amountRepaid);

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Rejecting wallet proposals is useless, as the proposal variables are not reset.

This bricks the contract functionality, as it's not possible to propose different wallets.

Proof of Concept

The mainWallet has the ability to propose new wallets, which are tracked in storage.

Confirmation wallets have the ability to confirm or reject those proposed wallets.

The problem is that the corresponding storage variables proposedMainWallet, and proposedConfirmationWallet are not reset to 0 when they are rejected:

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

        // Confirm if .05 or more ether is sent and otherwise reject.
        // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
        if (msg.value >= 0.05 ether) {
            activeTimelock = block.timestamp + TIMELOCK_DURATION;
        } // establish the timelock
        else {
            activeTimelock = type(uint256).max;
👉          // @audit proposals should be reset here
        } // effectively never
    }

ManagedWallet.sol#L58-L69

This bricks the contract functionality, as no new wallets can be proposed, since proposedMainWallet wasn't reset.

	function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external {

		// Make sure we're not overwriting a previous proposal (as only the confirmationWallet can reject proposals)
👉  	require( proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet." );

ManagedWallet.sol#L48-L49

Recommended Mitigation Steps

Reset proposedMainWallet, and proposedConfirmationWallet when rejecting wallet proposals:

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

        // Confirm if .05 or more ether is sent and otherwise reject.
        // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
        if (msg.value >= 0.05 ether) {
            activeTimelock = block.timestamp + TIMELOCK_DURATION;
        } // establish the timelock
        else {
            activeTimelock = type(uint256).max;
+           proposedMainWallet = address(0);
+           proposedConfirmationWallet = address(0);
        } // effectively never
    }

Assessed type

Other

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

L-8 - proposeWallets enters in a deadlock if the proposed wallet doesn’t call changeWallets() The mainWallet has the ability to propose new wallets, which are tracked in storage.

Confirmation wallets have the ability to confirm or reject those proposed wallets. When there is a confirmation, an active timelock is set.

Once the timelock is over, the proposed wallet can call changeWallets() to make the changes for real.

The problem is that if the proposed wallet doesn’t call the function, or can’t do it, it’s not possible for the proposer to propose a new one, because of this requirement.

Recommendation Allow the proposer to reset the timelock, and the proposed addresses if the proposed wallet didn’t call changeWallets() after some time.

Lines of code

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

Vulnerability details

Impact

Pools reserves can be manipulated to make the tokenB have as little as 1 wei in the reserves.

This is particularly dangerous when the liquidity is low, as in the case of recently whitelisted pools, where a malicious actor can manipulate the reserves as a first depositor, making the following users receive much less assets, or even prevent them from adding liquidity.

Proof of Concept

removeLiquidity() has a clear bug when checking the remaining reserves, as it checks twice the same comparison: reserves.reserve0 >= PoolUtils.DUST.

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

Pools.sol#L185-L187

This allows the first depositor to remove liquidity, up to the point where they leave just 1 wei of the reserve1.

The proportion of the reserves will be off when the next depositor tries to add liquidity, which will return much less tokens as it should.

Recommended Mitigation Steps

Replace reserve0 with reserve1 on the right side of the comparison:

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

An adversary can prevent legit proposals from being created by using the same ballot name.

Proposals with the same name can't be created, leading to a DOS for some days until the voting phase ends. This can be done repeatedly, after finalizing the previous malicious proposal and creating a new one.

Impacts for each proposal function:

• proposeSendSALT(): DOS of all proposals
• proposeSetContractAddress(): DOS of specific contract setting by proposing a malicious address
• proposeCallContract(): DOS of specific contract call by providing a wrong number
• proposeTokenWhitelisting(): DOS of token whitelisting by providing a fake tokenIconURL
• All: Prevent the creation of any legit proposal, by providing a fake/malicious description to discourage positive voting

Note: This Impact fits into the Attack Ideas: "Any issue that would prevent the DAO from functioning correctly."

Vulnerability Details

The main issue is that ballots with the same name revert, and the name doesn't contain all the important parameters to create the proposal:

// Make sure that a proposal of the same name is not already open for the ballot
require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );

Proposals.sol#L101-L102

proposeSendSALT() Vulnerability Details

Let's see for example the "Send SALT" proposal. It always has the same name sendSALT. Despite this appears to be an expected behaviour, it can be exploited by an adversary.

The minimum ballot duration is 3 days, with a default value of 10 days. Given that ballots can't be finalized before that, an adversary can consistently create malicious proposals to send themselves the SALT token. The proposal will enter the voting period for some days, and when the phase ends, the adversary can finalize it, and immediately create the same proposal.

This will prevent any other legit "Send SALT" proposal from being created.

There are no mechanisms to remove these malicious proposals, or to prevent malicious actors from creating them, nor removing their stake. The cost for the adversary is meaningless, as it requires to execute a tx every few days, and they can still claim rewards from the staked assets needed for the proposals.

proposeSetContractAddress() Vulnerability Details

Only the contractName is considered for the ballot name, but not the newAddress.

This means that an attack can be performed to consistently create proposals for a specific contract with a malicious address. This prevents updating the price feeds and the access manager.

proposeCallContract() Vulnerability Details

Only the contractName is considered for the ballot name, but not the number with which it will be called.

Same as with the previous attack, an adversary can target a specific contract, and consistently create proposals with a wrong calling number.

proposeTokenWhitelisting() Vulnerability Details

The tokenIconURL is missing in the ballot name, so whitelisting proposals can be maliciously created for a specific token with a wrong token icon.

description() Vulnerability Details

No proposal includes the description (or its hash) in its ballot name. So an adversary can prevent the creation of the legit proposal, by frontrunning it for example, and change the description to something that users would not vote for.

Proof of Concept

This test for proposeSendSALT() already shows how a new proposal can't be created when there is an existing one. An adversary can exploit that as explained on the Vulnerability Details section. That test could be extended to all the other mentioned functions with their corresponding impacts.

Recommended Mitigation Steps

In order to prevent the DOS, ballot names (or some new id variable) should include ALL the attributes of the proposal: ballotType, address1, number1, string1, and string2. Strings could be hashed, and the whole pack could be hashed as well.

So, if an adversary creates the proposal, it would look exactly the same as the legit one.

In the particular case of proposeSendSALT(), strictly preventing simultaneous proposals as they are right now will lead to the explained DOS. Some other mechanism should be implemented to mitigate risks. One way could be to set a long enough cooldown for each user, so that they can't repeatedly send these type of proposals (take into account unstake time).

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

An adversary can prevent the creation of setContract and websiteUpdate proposals by creating a poisonous proposal with the same ballot name + _confirm. This attack can be performed repeatedly every few days (when the voting period ends).

Affected proposals:

• setContract:priceFeed1 -> setContract:priceFeed1_confirm
• setContract:priceFeed2 -> setContract:priceFeed2_confirm
• setContract:priceFeed3 -> setContract:priceFeed3_confirm
• setContract:accessManager -> setContract:accessManager_confirm
• setURL:{{newWebsiteURL}} -> setURL:{{newWebsiteURL}}_confirm

This is especially worrisome for the setContract proposals, as it prevents changing the price feed contracts, which are used for USDS borrowing and liquidations.

Note: This Impact fits into the Attack Ideas: "Any issue that would prevent the DAO from functioning correctly."

Vulnerability Details

setContract and websiteUpdate proposals are executed in multiple steps.

Here's the process for changing the Price Feed 1, as an example:

1. A new proposal is created with a ballot name setContract:priceFeed1
2. The proposal is voted, and when it wins, a confirmation proposal is created, appending _confirm to the ballot name, resulting in setContract:priceFeed1_confirm.
3. When the confirmation proposal wins, it checks its ballot name and then it sets the new price feed.

The problem is that there is a check in _possiblyCreateProposal() that can be exploited:

require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

Proposals.sol#L103

This check is intended to prevent creating new proposals if there is a pending confirmation proposal being voted for the same change, but an adversary can use it to create a proposal with a ballot name setContract:priceFeed1_confirm, by setting priceFeed1_confirm as the contract name.

If someone tries to create a legit priceFeed1 proposal later, it will revert, leading to a DOS.

This holds for all proposals mentioned in the Impact section.

Proof of Concept

1. Add the test to src/dao/tests/Proposals.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://1rpc.io/sepolia --mt "testProposeSetContractAddress_confirm"

function testProposeSetContractAddress_confirm() public {
    uint256 initialNextBallotId = proposals.nextBallotID();

    // Alice is the adversary
    vm.startPrank(alice);
    staking.stakeSALT(1000 ether);

    // Create a poisonous proposal with a ballot name ending in `_confirm`
    address newAddress = address(0x1111111111111111111111111111111111111112);
    proposals.proposeSetContractAddress("priceFeed1_confirm", newAddress, "description");
    assertEq(proposals.nextBallotID(), initialNextBallotId + 1); // proposal was created successfully
    vm.stopPrank();

    // Transfer some tokens to Bob who wants to create a legit proposal
    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 1000 ether);
    vm.stopPrank();

    // Bob can't create a legit proposal to change the contract address of `priceFeed1`
    vm.startPrank(bob);
    staking.stakeSALT(1000 ether);
    vm.expectRevert("Cannot create a proposal for a ballot with a secondary confirmation");
    proposals.proposeSetContractAddress("priceFeed1", newAddress, "description");
}

Recommended Mitigation Steps

Given the current implementation, the most straightforward fix would be to prevent the creation of proposals with ballot names ending in _confirm for proposals that need a confirmation.

This would mean checking the contractName in proposeSetContractAddress(), and the newWebsiteURL in proposeWebsiteUpdate().

But, as a recommendation, I would suggest refactoring createConfirmationProposal() to pass a "confirmation" parameter to _possiblyCreateProposal(), so that confirmation proposals don't rely on the ballot name.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L280-L281 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L384-L385 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L395-L397

Vulnerability details

Impact

The current voting system specifies a minimum end time and forces each ballot to reach quorum before the proposal can be finalized. It also allows voting on the proposal after the minimal end time, with the same voting power as if done before that.

This creates a security risk, as malicious proposals may remain alive indefinitely after the minimal voting period ends, as long as there is no quorum.

It is important to note that the minimum quorum can reach values up to 60% of staked assets, which will make it hard to reach for every ballot at any moment in time. (60% calculated as 3X the max base ballot quorum percent).

Also, not only for malicious proposals, but for any proposal, the users that see that their option is losing can withdraw their votes, so that there is not enough quorum to finalize it. This way they can postpone the decision. They can also silently accumulate voting power, and flip the election later, filling the quorum gap.

Another point is that new proposals can't be created with the same ballot name. In some cases, that prevents creating some specific type of proposal altogether, like the "Send SALT" proposal. So, if a not so popular proposal doesn't reach quorum, it will block others, not being able to remove it, even when the minimum voting period ended.

Note: This Impact fits into the Attack Ideas: "Any issue that would prevent the DAO from functioning correctly."

Vulnerability Details

When trying to finalize a ballot, there is a check after evaluating if the minimum end time was reached, that asserts quorum was reached. If it returns false, the tx will revert.

// Check that the required quorum has been reached
if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
    return false;

Proposals.sol#L395-L397

This can be exploited as explained on the Impact section. Users can for example, remove their votes just before the minimum end time, to prevent reaching quorum, and add their voting power later, when they see they can reach quorum and flip the election.

In the meantime, proposals with the same ballot name can't be created, like sending SALT, or changing the address of a specific price feed for example.

Proof of Concept

1. Add this test to src/dao/tests/DAO.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://1rpc.io/sepolia --mt "testManipulatingQuorum"

function testManipulatingQuorum() public {
    // Alice creates a proposal
    vm.startPrank(alice);
    staking.stakeSALT(1000000 ether);
    address newAddress = address(0x1111111111111111111111111111111111111112);
    uint256 ballotID = proposals.proposeSetContractAddress("priceFeed1_confirm", newAddress, "description");

    // She casts her vote, at the same time increasing the votes needed to reach the quorum
    proposals.castVote(ballotID, Vote.YES);
    assertEq(proposals.totalVotesCastForBallot(ballotID), 1000000 ether);

    // The time passes and the ballot reaches its minimum end duration
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());

    // Alice realizes her option won't win the ballot
    // She can manipulate the quorum, by "withdrawing" her votes
    uint256 unstakeID = staking.unstake(1000000 ether - 100, 52);
    proposals.castVote(ballotID, Vote.YES);
    staking.cancelUnstake(unstakeID);

    // Now the quorum is reduced
    assertEq(proposals.totalVotesCastForBallot(ballotID), 100);

    // And the ballot can't be finalized
    vm.expectRevert("The ballot is not yet able to be finalized");
    dao.finalizeBallot(ballotID);

    // At any point in time in the future Alice can cast her vote again if she foresees that she can win
    // She may even stake more tokens to reach quorum + win the ballot
    staking.stakeSALT(1000000 ether);
    proposals.castVote(ballotID, Vote.YES);
    assertEq(proposals.totalVotesCastForBallot(ballotID), 2000000 ether);

    // And immediately finalize the ballot
    dao.finalizeBallot(ballotID);
}

Recommended Mitigation Steps

Allow ballots to be finalized without changes if they reach the minimum end time without quorum. Verify that any assumptions about quorum are not broken like in this case that assumes that "uorum would already have been determined".

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

In the event of a WBTC depeg, users can perform arbitrages against the protocol, as the actual price of WBTC will be lower than the protocol expects.

WBTC depeg is not theorical. It has already happened not so long ago, up to 2% down of the price of BTC. Ref#1 | Ref#2

Proof of Concept

The Chainlink price feed is using the BTC/USD price:

CHAINLINK_BTC_USD = AggregatorV3Interface(_CHAINLINK_BTC_USD);

CoreChainlinkFeed.sol#L21

As BTC is not native in Ethereum, a wrapped version WBTC is used in the protocol.

The problem is that WBTC can depeg from BTC, as shown on the links in the Impact section.

This means that in case of a depeg the getPriceBTC() will continue to report the same price (despite the WBTC token will actually have a lower price).

This is used by the price aggregator, which will report an incorrect price to the CollateralAndLiquidity function to get the underlying value in USD.

The protocol will consider that the collateral is worth more than its real value, allowing arbitragers to:

• Borrow more USDS than they should
• Make positions more difficult to liquidate
• Allow users to withdraw more collateral than they should

Recommended Mitigation Steps

Calculate the actual price of WBTC, combining WBTC/BTC + BTC/USD feeds.

Have in mind the differences in heartbeat, deviation, and decimals of the WBTC/BTC feed.

Assessed type

Oracle

QA Report

Low Severity Findings

L-1 - callContract proposals are created with no description

Impact

callContract proposals will be created with no description. The Proposal contract will be bricked, as it can't be fixed on-chain. Off-chain tools and integrations will have to create special code to lead with this error. Integrating contracts will have to take this error into account as well.

Vulnerability Details

Note how description is placed on the string1 parameter of _possiblyCreateProposal for callContract proposals. string2 is actually used for the description, so its actual value here will be "".

    // Proposes calling the callFromDAO(uint256) function on an arbitrary contract.
    function proposeCallContract( address contractAddress, uint256 number, string calldata description ) external nonReentrant returns (uint256 ballotID) {
        require( contractAddress != address(0), "Contract address cannot be address(0)" );

        string memory ballotName = string.concat("callContract:", Strings.toHexString(address(contractAddress)) );
👉      return _possiblyCreateProposal( ballotName, BallotType.CALL_CONTRACT, contractAddress, number, description, "" );
    }

string1 is used for calling attributes everywhere else, and string2 is used instead for the description, like in createConfirmationProposal(), proposeParameterBallot(), proposeTokenWhitelisting() just to name a few, but it holds for all proposals.

Recommended Mitigation Steps

Fix the code like this:

    // Proposes calling the callFromDAO(uint256) function on an arbitrary contract.
    function proposeCallContract( address contractAddress, uint256 number, string calldata description ) external nonReentrant returns (uint256 ballotID) {
        require( contractAddress != address(0), "Contract address cannot be address(0)" );

        string memory ballotName = string.concat("callContract:", Strings.toHexString(address(contractAddress)) );
-       return _possiblyCreateProposal( ballotName, BallotType.CALL_CONTRACT, contractAddress, number, description, "" );
+       return _possiblyCreateProposal( ballotName, BallotType.CALL_CONTRACT, contractAddress, number, "", description );
    }

L-2 - withdrawArbitrageProfits() will revert when depositedWETH <= PoolUtils.DUST

withdrawArbitrageProfits() will revert when attempting to withdraw less than the dust amount, because of a check on the pool.

This is called in step 2 of the Upkeep.

Recommendation

Safely return when the depositedWETH is less than the pools dust:

    // The arbitrage profits are deposited in the Pools contract as WETH and owned by the DAO.
    uint256 depositedWETH = pools.depositedUserBalance(address(this), weth );
-   if ( depositedWETH == 0 )
+   if ( depositedWETH <= PoolUtils.DUST )
        return 0;

    pools.withdraw( weth, depositedWETH );

L-3 - string1 and string2 length is not validated when creating proposals

In _possiblyCreateProposal() string1 length is not validated for tokenIconURL or newWebsiteURL. string2 is not validated for the proposal description.

Very large strings will make proposals difficult to read for voters.

Recommendation

Set a limit to the length of the strings

L-4 - It is possible to create proposals that can't be finalized

Certain attributes make passing proposal impossible to be finalized, as they will revert. This also bricks the ability of the proposer to ever propose another ballot again, and prevents other users from creating a new ballot with the same name.

Affected proposals:

• proposeParameterBallot() allows to set any parameterType, but it can revert when trying to convert it to ParameterTypes, if the number is ot of bounds for the ParameterTypes enum.

Recommendation

Verify the range of the attributes on proposal creation.

L-5 - tokenWhitelistingBallotWithTheMostVotes() returns the ballot id 0 when no proposal reaches quorum

If no proposal votes reaches quorum, then the function will returns its default value, which is 0.

Recommendation

Revert when the returned value is 0. It is safe, as ballotId starts at 1, and when the function is used to finalize whitelisted tokens, the ballot already needs to reach quorum to get to this point.

L-6 - websiteURL should be set in the constructor

The websiteURL is set on the DAO only after a ballot voting passes. The initial setup will take many days, from airdrop, to staking, to proposing, to voting, leaving the DAO contract with no initial website URL during that period.

Recommendation

Set an initial value for websiteURL in the DAO constructor.

L-7 - Votes can't be delegated

The protocol is missing some function to delegate votes to other users.

This is important as not all users will participate in DAO voting (as there is no direct incentive), and those votes could be delegated to other users they trust and will be actively participating in the DAO.

This makes it easier to reach quorum, to prevent malicious proposals from passing, and incentivizes participation of dedicated users in the DAO.

L-8 - proposeWallets enters in a deadlock if the proposed wallet doesn't call changeWallets()

The mainWallet has the ability to propose new wallets, which are tracked in storage.

Confirmation wallets have the ability to confirm or reject those proposed wallets. When there is a confirmation, an active timelock is set.

Once the timelock is over, the proposed wallet can call changeWallets() to make the changes for real.

The problem is that if the proposed wallet doesn't call the function, or can't do it, it's not possible for the proposer to propose a new one, because of this requirement.

Recommendation

Allow the proposer to reset the timelock, and the proposed addresses if the proposed wallet didn't call changeWallets() after some time.

Non-Critical Issues

L-9 - Replace == balance comparison with >=

Even if the initial distribution is supposed to have an exact number of tokens, it is safer to check that the function to distribute them won't revert if for any reason it has more tokens, which would lock them in the contract:

-   require( salt.balanceOf(address(this)) == 100 * MILLION_ETHER, "SALT has already been sent from the contract" );
+   require( salt.balanceOf(address(this)) >= 100 * MILLION_ETHER, "SALT has already been sent from the contract" );

InitialDistribution.sol#L53

L-10 - Signatures do not implement EIP-712

Impact

EIP-712 Motivation is to improve the usability of off-chain message signing for use on-chain.

By not adhering to it, users will find more difficulty understanding what they are signing, as off-chain tools will have trouble decoding the messsage, leading to a compatibility and integration issue.

Signatures can also be replayed on other contracts, as the contract address is not included in the message hash.

Proof of Concept

Message hashes are implemented without following specs by EIP-712, missing the typeHash, and the domainSeparator.

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

AccessManager.sol#L53

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

BootstrapBallot.sol#L53-L54

Recommendation

Follow the EIP-712 spec, including the corresponding typeHash, domainSeparator, contract address, and build the message hash accordingly.

L-11 - DUST value too big for tokens with low decimals

Some tokens with a considerable market cap like GUSD, or EURS have 2 decimals.

The DUST value is fixed at 100, which can be considerable big as dust, since it is used all over the protocol, with many require depending on the amount being bigger than it.

Recommendation

Consider defining dust values considering proportional to the decimals of the tokens

L-12 - Arbitrage profit is not calculated correctly when there is an index with an INVALID_POOL_ID

Impact

1/3 to 2/3 of arbitrage profit may not be distributed

Proof of Concept

Arbitrage profit is always divided by 3, despite some indicies might have an INVALID_POOL_ID. That means if an index has an invalid pool id, that 33% of arbitrage profits will not be distributed.

    // Split the arbitrage profit between all the pools that contributed to generating the arbitrage for the referenced pool.
👉  uint256 arbitrageProfit = _arbitrageProfits[poolID] / 3;
    if ( arbitrageProfit > 0 )
        {
        ArbitrageIndicies memory indicies = _arbitrageIndicies[poolID];

        if ( indicies.index1 != INVALID_POOL_ID )
👉          _calculatedProfits[indicies.index1] += arbitrageProfit;

        if ( indicies.index2 != INVALID_POOL_ID )
👉          _calculatedProfits[indicies.index2] += arbitrageProfit;

        if ( indicies.index3 != INVALID_POOL_ID )
👉          _calculatedProfits[indicies.index3] += arbitrageProfit;
        }
    }

PoolStats.sol#L111-L126

Recommendation

Count the number of indicies without an INVALID_POOL_ID, and then distribute the profit evenly among them

L-13 - Users from excluded countries, without wallet authorization can call cancelUnstake()

There is no restriction in cancelUnstake() to prevent wallets without access to call the function, like in stakeSALT().

Canceling the unstake process puts back the staked SALT in the contract, like with the staking process.

Recommendation

Consider not allowing unauthorized users to cancel their unstake process.

    function cancelUnstake( uint256 unstakeID ) external nonReentrant
+       require( exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access" );

L-14 - sum in RewardsEmitter is not used

The sum variable is calculated in the RewardsEmitter but never used, nor returned.

It may have been left from a previous refactor, as a sum variable is also used in StakingRewards::addSALTRewards(), which is called by the RewardsEmitter.

The RewardsEmitter already approved the SALT token to its maximum, so there should be no issue. On any case, I'm highlighting this, as it may hide some other issue I couldn't detect.

Recommendation

Verify that sum is actually not used, and remove it from the RewardsEmitter contract if not.

NC-1 - tokenIconURL is not used in proposeTokenUnwhitelisting()

tokenIconURL is only used when whitelisting tokens but not when unwhitelisting.

Recommendation

Consider removing it from proposeTokenUnwhitelisting()