Salty.IO - zhaojie's results

Lines of code

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

Vulnerability details

The first user of pool gets all of the StakingRewards

Impact

The first user of pool gets all of the StakingRewards.

Proof of Concept

StakingRewards#_increaseUserShare if totalShares are 0, user.virtualRewards is not increased:

	function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal{
        .....
        uint256 existingTotalShares = totalShares[poolID];
        if ( existingTotalShares != 0 ) // prevent / 0
        	{
			// Round up in favor of the protocol.
			uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );
			user.virtualRewards += uint128(virtualRewardsToAdd);
	        totalRewards[poolID] += uint128(virtualRewardsToAdd);
	        }
		// Update the deposit balances
		user.userShare += uint128(increaseShareAmount);
		totalShares[poolID] = existingTotalShares + increaseShareAmount;
    }

Calculate the userReward need and subtract user.virtualRewards:

	function userRewardForPool( address wallet, bytes32 poolID ) public view returns (uint256)
    {
        .....
        // Determine the share of the rewards for the user based on their deposited share
        uint256 rewardsShare = ( totalRewards[poolID] * user.userShare ) / totalShares[poolID];

        // Reduce by the virtualRewards - as they were only added to keep the share / rewards ratio the same when the used added their share

        // In the event that virtualRewards exceeds rewardsShare due to precision loss - just return zero
        if ( user.virtualRewards > rewardsShare )
            return 0;

        return rewardsShare - user.virtualRewards;
    }

Therefore, if user.virtualRewards is equal to 0, the userRewardForPool function returns an incorrect value.

After testing: If the totalRewards are initialized with 1000 ether and the first user Staking 1 ether, then the first user will get 1000 ether Staking Rewards.

In the DAO, when we create a pool, will add SALTRewards:

	function _finalizeTokenWhitelisting( uint256 ballotID ) internal
		{
		if ( proposals.ballotIsApproved(ballotID ) )
			{
            ......
			// 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 );

			exchangeConfig.salt().approve( address(liquidityRewardsEmitter), bootstrappingRewards * 2 );
			liquidityRewardsEmitter.addSALTRewards( addedRewards );

			emit WhitelistToken(IERC20(ballot.address1));
			}

		// Mark the ballot as finalized (which will also remove it from the list of open token whitelisting proposals)
		proposals.markBallotAsFinalized(ballotID);
		}

Both the Staking module and the Liquidity module call _increaseUserShare function, Staking#stakeSALT Liquidity#depositLiquidityAndIncreaseShare CollateralAndLiquidity#depositCollateralAndIncreaseShare

Test code:

    function testUserRewardsFirst() public {
        AddedReward[] memory addedRewards = new AddedReward[](2);
        addedRewards[0] = AddedReward(poolIDs[0], 1000 ether);
        staking.addSALTRewards(addedRewards);

        vm.prank(alice);
        staking.stakeSALT(1 ether);

        console.log("alice:");
        console.log(staking.userXSalt(alice) /1 ether);
        console.log(staking.userVirtualRewardsForPool(alice, PoolUtils.STAKED_SALT));
        console.log(staking.userRewardForPool(alice, PoolUtils.STAKED_SALT)/1 ether);
    }

Put the test code in Staking.t.sol and run test, we can see from the log that alice's userReward is 1000 ether

[PASS] testUserRewardsFirst() (gas: 172593)
Logs:
  alice:
  1
  0
  1000

Tools Used

vscode, manual

Recommended Mitigation Steps

If existingTotalShares is equal to 0, you also set user.virtualRewards.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The first user deposits 1 wei to the pool to attack the pool.

Proof of Concept

The calculations of virtualRewardsToAdd in the _increaseUserShare function are as follows:

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

Thus, when existingTotalShares equal 1, virtualRewardsToAdd becomes large. totalRewards[poolID], which also becomes a large value.

But the problem is, userReward is calculated by dividing by totalShares[poolID], totalShares is a very small value, So when the totalRewards increase, the userReward increase very quickly

    function userRewardForPool( address wallet, bytes32 poolID ) public view returns (uint256)
		{
        ....
		uint256 rewardsShare = ( totalRewards[poolID] * user.userShare ) / totalShares[poolID];
		if ( user.virtualRewards > rewardsShare )
			return 0;
		return rewardsShare - user.virtualRewards;
		}

The test scenario is as follows:

1. SALTRewards with 1000 ether is added during pool initialization.
2. The first user, Alice, deposits 1 wei
3. The second user, Bob, deposits 1 ether
4. The third user, Charlie, deposits 2 ether
5. Now Charlie's userReward is 113427455640312821230 ether!

  function testUserRewards11() public {
        AddedReward[] memory addedRewards = new AddedReward[](2);
        addedRewards[0] = AddedReward(poolIDs[0], 1000 ether);
        staking.addSALTRewards(addedRewards);

        vm.prank(alice);
        staking.stakeSALT(1);

        console.log("alice:");
        console.log(staking.userXSalt(alice));
        console.log(staking.userVirtualRewardsForPool(alice, PoolUtils.STAKED_SALT));
        console.log(staking.userRewardForPool(alice, PoolUtils.STAKED_SALT)/1 ether);

        vm.prank(bob);
        staking.stakeSALT(1 ether);

        console.log("bob:");
        console.log(staking.userXSalt(bob));
        console.log(staking.userVirtualRewardsForPool(bob, PoolUtils.STAKED_SALT));
        console.log(staking.userRewardForPool(bob, PoolUtils.STAKED_SALT)/1 ether);

        vm.prank(charlie);
        staking.stakeSALT(2 ether);
                
        console.log("charlie:");
        console.log(staking.userXSalt(charlie));
        console.log(staking.userVirtualRewardsForPool(charlie, PoolUtils.STAKED_SALT));
        console.log(staking.userRewardForPool(charlie, PoolUtils.STAKED_SALT)/1 ether);
    }

Running 1 test for src/staking/tests/Staking.t.sol:StakingTest
[PASS] testUserRewards11() (gas: 285826)
Logs:
  alice:
  1
  0
  1000
  bob:
  1000000000000000000
  319435266158123073073250785136463577088
  680
  charlie:
  2000000000000000000
  298588165395307684044256430524912795213
  113427455640312821230

Tools Used

vscode, manual

Recommended Mitigation Steps

Limit the minimum deposit amount.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Malicious users can make their debts unliquidated

Proof of Concept

In the CollateralAndLiquidity#liquidateUser function, when a user needs to be liquidated, use the _decreaseUserShare function to reduce the user's userShare. The problem is that the last argument to this function, useCooldown = true, will have a time lock.

	function liquidateUser( address wallet ) external nonReentrant{
		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
    }

    function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal{
		require( decreaseShareAmount != 0, "Cannot decrease zero share" );

		UserShareInfo storage user = _userShareInfo[wallet][poolID];
		require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );

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

			// Update the cooldown expiration for future transactions
			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
			}
        }
        ....
    }

Therefore, if the user is in the cooldown state, it will not be liquidated.

Because, despdepositCollateralAndIncreaseShare and withdrawCollateralAndClaim will reset the time lock, Therefore, the malicious user only needs to call despdeposit/withdraw a small amount of collateral to reset cooldown when he finds out that he is about to be liquidated, so that the user can never be liquidated.

	function _depositLiquidityAndIncreaseShare(....){
        ......
		_increaseUserShare( msg.sender, poolID, addedLiquidity, true );
        ......
	}

    function _withdrawLiquidityAndClaim(.....) {
		......
		_decreaseUserShare( msg.sender, poolID, liquidityToWithdraw, true );
        ......
	}

Tools Used

vscode, manual

Recommended Mitigation Steps

	function liquidateUser( address wallet ) external nonReentrant{
-		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
+		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );
    }

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The attacker made proposeTokenWhitelisting impossible to create.

Proof of Concept

	function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
	{
		require( address(token) != address(0), "token cannot be address(0)" );
		require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" ); // 5 quadrillion max supply with 18 decimals of precision

@>		require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
		require( ! poolsConfig.tokenHasBeenWhitelisted(token, exchangeConfig.wbtc(), exchangeConfig.weth()), "The token has already been whitelisted" );

		string memory ballotName = string.concat("whitelist:", Strings.toHexString(address(token)) );

		uint256 ballotID = _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );
@>		_openBallotsForTokenWhitelisting.add( ballotID );
		return ballotID;
	}

ProposeTokenWhitelisting will check _openBallotsForTokenWhitelisting.length() whether more than daoConfig.maxPendingTokensForWhitelisting. But the question is anyone can call this function, the attacker can create invalid propose, let _openBallotsForTokenWhitelisting. Length more than the maximum value. proposeTokenWhitelisting could no longer be executed.

Because the daoConfig.maxPendingTokensForWhitelisting defaults to 5, so _openBallotsForTokenWhitelisting. The length is more than the maximum easily.

	uint256 public maxPendingTokensForWhitelisting = 5;

Tools Used

vscode, manual

Recommended Mitigation Steps

	function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
	{
		require( address(token) != address(0), "token cannot be address(0)" );
		require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" ); // 5 quadrillion max supply with 18 decimals of precision

-		require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
		require( ! poolsConfig.tokenHasBeenWhitelisted(token, exchangeConfig.wbtc(), exchangeConfig.weth()), "The token has already been whitelisted" );

		string memory ballotName = string.concat("whitelist:", Strings.toHexString(address(token)) );

		uint256 ballotID = _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );
		_openBallotsForTokenWhitelisting.add( ballotID );
		return ballotID;
	}

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When the locked salt token expires, the user can unlock the token, transfer it to another account, and vote again after staking.

Proof of Concept

Votes in the DAO, staking salt token to get votingPower:

	function castVote( uint256 ballotID, Vote vote ) external nonReentrant
		{
        .....
@>		uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
		require( userVotingPower > 0, "Staked SALT required to vote" );

		// Remove any previous votes made by the user on the ballot
		UserVote memory lastVote = _lastUserVoteForBallot[ballotID][msg.sender];

		// Undo the last vote?
		if ( lastVote.votingPower > 0 )
			_votesCastForBallot[ballotID][lastVote.vote] -= lastVote.votingPower;

		// Update the votes cast for the ballot with the user's current voting power
		_votesCastForBallot[ballotID][vote] += userVotingPower;

		// Remember how the user voted in case they change their vote later
		_lastUserVoteForBallot[ballotID][msg.sender] = UserVote( vote, userVotingPower );

		emit VoteCast(msg.sender, ballotID, vote, userVotingPower);
		}

Since staking salt tokens require a certain amount of time to be unlocked, they cannot be immediately unstaking, However, a malicious user can unlock the token at expiration to get double the votingPower.

1. Alice locks the salt token votingPower.
2. After a certain period of time,Alice's time lock is almost current, and Alice can unlock the token.
3. There is a vote going on.
4. Alice does not unlock the token
5. Alice took a vote.
6. Alice unlocks the token.
7. Alice sends the salt token to Bob.
8. Bob locks the token and obtains votingPower.
9. Bob votes again.

Tools Used

vscode, manual

Recommended Mitigation Steps

Use the snapshot feature when voting, such as using openzeppelin's governance library.

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

requiredQuorum may be lower than the actual value, it is possible that the vote will pass early.

Proof of Concept

A canFinalizeBallot is used to determine whether a quorum is present.

	function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
		{
        Ballot memory ballot = ballots[ballotID];
        if ( ! ballot.ballotIsLive )
        	return false;

        // Check that the minimum duration has passed
        if (block.timestamp < ballot.ballotMinimumEndTime )
            return false;

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

        return true;
	    }

requiredQuorumForBallotType through salt token to calculate the total supply minimumQuorum:

	function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
		{
		....
		uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
		uint256 minimumQuorum = totalSupply * 5 / 1000;

		if ( requiredQuorum < minimumQuorum )
			requiredQuorum = minimumQuorum;
		}

The problem is that totalSupply is not a fixed value, and minimumQuorum is lower than the actual value when totalSupply decreases:

    function burnTokensInContract() external
    {
    	uint256 balance = balanceOf( address(this) );
    	_burn( address(this), balance );

    	emit SALTBurned(balance);
    }

Tools Used

vscode, manual

Recommended Mitigation Steps

Use the snapshot feature when voting, such as using openzeppelin's governance library.

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

The attacker uses front-running to prevent the specified Proposal from being created.

Proof of Concept

	function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
	{
        ......
		require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
        .....
	}

The Proposal is created by checking whether ballotName already exists in openBallotsByName, and if it does, it cannot be created.

The problem is that if an attacker creates a Proposal with this ballotName in advance, the Proposal cannot be created.

openBallotsByName[ballotName], is not removed until the vote is complete, so a Proposal created by an attacker will never be removed if no one votes on it.

	function markBallotAsFinalized( uint256 ballotID ) external nonReentrant{
        ......
        delete openBallotsByName[ballot.ballotName];
        ......
    }

For example, proposeParameterBallot, anything can be called:

	function proposeParameterBallot( uint256 parameterType, string calldata description ) external nonReentrant returns (uint256 ballotID)
	{
		string memory ballotName = string.concat("parameter:", Strings.toString(parameterType) );
		return _possiblyCreateProposal( ballotName, BallotType.PARAMETER, address(0), parameterType, "", description );
	}

The attacker finds that there is a proposeParameterBallot to be executed. The attacker executes the same function, uses the same parameterType parameter, keeps ballotName consistent, uses any description, and passes front-running Make the transaction execute in advance so that the compromised proposeParameterBallot cannot be successfully created.

Tools Used

vscode, manual

Recommended Mitigation Steps

hash the other parameters of the Ballot to determine the uniqueness of the Ballot, not by name.

Assessed type

DoS