Salty.IO - grearlake's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L259-#L293 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L60-#L79

Vulnerability details

Vulnerability detail

When cast vote on a open ballot, user's vote is calculated based on user's current share of salt:

  uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );

When user unstake and decrease shares, these voting power is not decreased, as there is no mechanism to do that:

function unstake( uint256 amountUnstaked, uint256 numWeeks ) external nonReentrant returns (uint256 unstakeID)//@audit đánh giá sự thay đổi khi unstake -> update config -> xxx
	{
	require( userShareForPool(msg.sender, PoolUtils.STAKED_SALT) >= amountUnstaked, "Cannot unstake more than the amount staked" );

	uint256 claimableSALT = calculateUnstake( amountUnstaked, numWeeks );
	uint256 completionTime = block.timestamp + numWeeks * ( 1 weeks );

	unstakeID = nextUnstakeID++;
	Unstake memory u = Unstake( UnstakeState.PENDING, msg.sender, amountUnstaked, claimableSALT, completionTime, unstakeID );

	_unstakesByID[unstakeID] = u;
	_userUnstakeIDs[msg.sender].push( unstakeID );

	// Decrease the user's staking share so that they will receive less future SALT rewards
	// This call will send any pending SALT rewards to msg.sender as well.
	// Note: _decreaseUserShare checks to make sure that the user has the specified staking share balance.
	_decreaseUserShare( msg.sender, PoolUtils.STAKED_SALT, amountUnstaked, false );//@auditz stake & unstake không dùng cooldown (nó có cooldown trên kia kìa)

	emit UnstakeInitiated(msg.sender, unstakeID, amountUnstaked, claimableSALT, numWeeks);
	}

Attacker can abusing this to increase voting for free by keep voting, unstake, then vote again. This can be abused by attack to execute malicious actions. A way to execute malicious action is propose CALL_CONTRACT to malicious address that delegate call to multiple function that have onlyOwner function, like whitelistPools(), unwhitelistPool(), ....

Moreover, there is no expire for ballot, it will only check if ballot is executed before, passed minimum end time and it pass required quorum or not:

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;
    }

Attack scenario:

• Attacker call proposeCallContract with input is malicious contract address
• Attacker vote for that proposal
• Unstake salt token
• Stake again by using another address have staking access
• Cast vote for that ballot again
• Repeat this process until vote is enough

This process can be extended for years, depend on number of vote attacker have and unstaking period. But when it is success, impact is very huge, since all functions that have onlyOwner can be delegate call from that malicious contract.

Impact

Attacker can freely edit configurations of many variable. But the most dangerous is, attacker can unwhitelist any token that restricted when calling proposeTokenUnwhitelisting() function, lead to protocol break. Moreover, attacker can whitelist multiple pools with their malicious token that only owned by them to steal salt rewards

Tools Used

Manual review

Recommended Mitigation Steps

There are multiple thing that need to fix:

• Ballot should have expire time
• When user unstake salt, all previous voting power should be loss
• Propose to call to arbitrary contract should be deleted, because even when these 2 solution above is mitigrated, it still have risk that anyone have 51% voting power can control this protocol

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreUniswapFeed.sol#L50-#L72

Vulnerability details

Vulnerability Detail

In CoreUniswapFeed contract, function _getUniswapTwapWei() is used by protocol to get average price (in last 30 minutes).

The problem is that in case if tickCumulatives[1] - tickCumulatives[0] is negative, then timeWeightedTick should be rounded down like Uniswap library

As result, in case if tickCumulatives[1] - tickCumulatives[0] is negative and (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, tick will be bigger then it should be. Which opens possibility for arbitrage opportunities.

Impact

If tickCumulatives[1] - tickCumulatives[0] is negative and ((tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, then returned tick will be bigger than it should be.

Tools Used

Manual review

Recommended Mitigation Steps

Tick should be rounded down in that case:

    int24 tick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(uint56(twapInterval)));
 +  if ((tickCumulatives[1] - tickCumulatives[0]) < 0 && ((tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0)) tick--;

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreChainlinkFeed.sol#L1

Vulnerability details

Vulnerability details

The chainlink BTC/USD oracle is used to price WBTC (docs). WBTC is basically a bridged asset and if the bridge is compromised/fails then WBTC will depeg and will no longer be equivalent to BTC. This will break one oracle data source, since there is no way to change chainlink oracle address

Impact

When WBTC bridge become compromised and WBTC depegs, protocol will lost one source to fetch oracle data, which make risk that price can be manipulated become higher

Tools Used

Manual review

Recommended Mitigation Steps

WBTC/USD chainlink feed should be used instead of BTC/USD

Assessed type

Oracle

1, User might unintentionally vote for other ballot when reorg happen

Link to code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L108C3-L108C29

Vulnerability details

When creating ballot, ballotId is created with linear number pattern:

   ballotID = nextBallotID++;

It will lead to scenario that when user create proposal and vote for it, when reorg happen, attacker can create malicious ballot with same type and receive that vote. But since user can undo vote, and it require ballot need to pass minimum time to be able to be approved, along with chance that reorg happen in the mainnet is very small, so it is very hard to make this attack success

Mitigration steps

BallotId should be generated by hashing proposer address and ballotId

---

2, approve() in _depositLiquidityAndIncreaseShare() does not reset to 0 after transfer, future approve can be reverted

Link to code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L83-#L117

Vulnerability details

To make Pools contract able to use token in the contract, approve() is used:

	tokenA.approve( address(pools), maxAmountA );
	tokenB.approve( address(pools), maxAmountB );

But it might not use all of them, so the unused part will be refunded to the user:

	if ( addedAmountA < maxAmountA )
		tokenA.safeTransfer( msg.sender, maxAmountA - addedAmountA );

	if ( addedAmountB < maxAmountB )
		tokenB.safeTransfer( msg.sender, maxAmountB - addedAmountB );

Some tokens, like USDT, will revert new approve if allowance is not zero, it will lead to function revert.

Mitigration steps

Approve to zero after calling pools.addLiquidity() function