Salty.IO - jesjupyter's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L409-L419 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreSaltyFeed.sol#L32-L54

Vulnerability details

Impact

The Pool::getPoolReserves returns the reserve0 && reserve1 in real-time, and no TWAP is provided in the contract. However, if the function is later used in CoreSaltyFeed::getPriceBTC or CoreSaltyFeed::getPriceETH or other contracts to feed price of tokens, a flashloan attack could be performed before-head to manipulate the ratio and thus the price.

Proof of Concept

Consider the following scenario:

1. There's a pool pair with 2 tokens, let's name it TOKEN-A and USDT.
2. An exterior contract Ex (Ex. Lending Pool) retrieves the TOKEN-A/USDT ratio for price-feeding since there is no TWAP provided.
3. The attacker could use Flashloan and call Pool::swap to buy TOKEN-A with USDT to increase the price of TOKEN-A. Thus, he may call Ex to borrow USDT with TOKEN-A as the collateral. Since the price is manipulated, he may borrow much more from Ex.

Tools Used

VSCode, Manual

Recommended Mitigation Steps

1. Provide another TWAP price that is more resistant to Flashloan manipulation which is also implemented by Uniswap
2. Specify this vulnerability in the DOC or comment.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L174 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L138 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L272 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAOConfig.sol#L47-L53 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L95

Vulnerability details

Impact

Malicious Users can fill _openBallotsForTokenWhitelisting with customized/fake/malicious/low-valued tokens by front-running others calling proposeTokenWhitelisting. Even though these ballots would not be approved and would be removed from _openBallotsForTokenWhitelisting due to the help from the DAO community after ballot.ballotMinimumEndTime(at least 10 days) has passed, this could prevent the normal call of proposeTokenWhitelisting and prevent the DEX from working.

Proof of Concept

In the function Proposals::proposeTokenWhitelisting, there is no access control and anyone could propose his token. If the _openBallotsForTokenWhitelisting has space and token satisfies the totalSupply requirement, the token is then added to the _openBallotsForTokenWhitelisting.

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

If the user front-runs others with multiple qualified wallets and calls proposeTokenWhitelisting, others won't be able to propose their tokens. And the _openBallotsForTokenWhitelisting could be filled with customized/fake/malicious/low-valued tokens. Thus the DEX is unable to whitelist high-quality tokens.

For a proposal, When its ballot.ballotMinimumEndTime has passed, consider two scenarios:

• if the ballot is approved, the proposed token would be whitelisted, and malicious users could make profits or even rug-pull which may cause damage to normal users.
• if the ballot is rejected, the malicious user could front-run again and propose the token or other token to fill the proposeTokenWhitelisting.

Even though, there's a restriction that the user could only have one active proposal and the user should stake some SALT to propose. However, considering what is required to attack with default setting, worst-case setting, and best-case setting, it is still highly likely that the attack could be conducted against the community.

• By default, it requires 0.5% percentage of totalStaked SALT (requiredProposalPercentStakeTimes1000 = 500), and the default maxPendingTokensForWhitelisting is 5, it only requires 2.5% of totalStaked SALT to perform the attack. -In the worst case, it only requires 0.1% percentage of totalStaked SALT, and the default maxPendingTokensForWhitelisting is 3, it only requires 0.3% of totalStaked SALT to perform the attack. -In the best case, it requires 2% percentage of totalStaked SALT, and the default maxPendingTokensForWhitelisting is 12, it requires 24% of totalStaked SALT to perform the attack.

Tools Used

Manual, VSCode

Recommended Mitigation Steps

To mitigate this issue, I think there are possibly a few ways that can help.

1. Tokens that have been proposed before could not be proposed again.
2. A blacklist mechanism could be set to prevent malicious users from sending proposals again.
3. If a proposal is rejected with overwhelming power, the DAO coulåd punish the proposer by burning a portion of his XSALT.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The changing mechanism of ManagedWallet requires a proposal and confirmation. The value activeTimelock is updated each time the receive function is triggered. However, when a new proposeWallets is rejected (for example, wrong address input by mistake), the activeTimelock is reset to type(uint256).max but the proposedMainWallet and proposedConfirmationWallet is not reset. So, the changeWallets can't be called due to block.timestamp >= activeTimelock and new proposal can't be called due to require( proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet." );. Thus the mechanism gets stuck.

Proof of Concept

Consider the following situation.

1. The mainWallet wants to make some modifications, and he calls proposeWallets.
2. The confirmationWallet rejects the proposal by sending 0.01 ETH to the contract.
3. No one can call changeWallets.
4. The mainWallet can't call proposeWallets.

POC below:

    function setUp() public {
        managedWallet = new ManagedWallet(alice,bob);
        vm.deal(alice, 10 ether);
        vm.deal(bob, 10 ether);

    }

    function test_walletBypass() public {
        vm.prank(alice);
        managedWallet.proposeWallets(zed,bob);
        vm.prank(bob);
        address(managedWallet).call{value : 0.01 ether}("");
        console.log("Lock time = > ", managedWallet.activeTimelock());
        vm.warp(block.timestamp + 31 days);
        vm.startPrank(zed);
        vm.expectRevert("Timelock not yet completed");
        managedWallet.changeWallets();        
        vm.startPrank(alice);
        vm.expectRevert("Cannot overwrite non-zero proposed mainWallet.");
        managedWallet.proposeWallets(zed,zed);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Modify receive function: when the proposal is rejected, reset proposedMainWallet and proposedConfirmationWallet to address(0).

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

It's clear that the require statement here is incorrect which should be require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");. By making reserves.reserve1 < PoolUtils.DUST and reserves.reserve0 >= PoolUtils.DUST, the original check could be bypassed.

Proof of Concept

In the code comments, it says: "// Make sure that removing liquidity doesn't drive either of the reserves below DUST." However, the require statement incorrectly uses reserves.reserve0 >= PoolUtils.DUST twice which violates the meaning of the DOC.

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

By making reserves.reserve1 < PoolUtils.DUST and reserves.reserve0 >= PoolUtils.DUST, the original check could be bypassed.

Tools Used

Manual

Recommended Mitigation Steps

Change the require statement to

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

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L51-L55 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L200 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L127-L130 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L134-L140 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L196

Vulnerability details

Impact

When a pool is unwhitelisted, it is removed from _whitelist, however when later the upkeep is performed by Upkeep::performUpkeep, the _arbitrageProfits information of all whitelisted pools will be cleared. But when the removed pool is later get whitelisted, its _arbitrageProfits will remain unchanged which will cause inconsistency and misunderstanding when PoolStats::profitsForWhitelistedPools is being called. Furthermore, since Upkeep uses profitsForWhitelistedPools to distribute rewards for pools, thus the pool will get more rewards than expected.

Proof of Concept

When PoolStats::clearProfitsForPools is being called, all whitelisted pools' _arbitrageProfits will be cleared.

function clearProfitsForPools() external
    {
    require(msg.sender == address(exchangeConfig.upkeep()), "PoolStats.clearProfitsForPools is only callable from the Upkeep contract" );

    bytes32[] memory poolIDs = poolsConfig.whitelistedPools();

    for( uint256 i = 0; i < poolIDs.length; i++ )
        _arbitrageProfits[ poolIDs[i] ] = 0;
    }

However, if a pool is unwhitelisted before the clearProfitsForPools call, and added back to _whitelist afterwards, its _arbitrageProfits would remain to be the value before. Thus this is inconsistent with the design of performUpkeep and could potentially cause misunderstanding and wrong statistics when PoolStats::profitsForWhitelistedPools is being called to Look at the arbitrage that has been generated since the last performUpkeep and determine how much each of the pools contributed to those generated profits.

// Look at the arbitrage that has been generated since the last performUpkeep and determine how much each of the pools contributed to those generated profits.
// Returns the profits for all of the current whitelisted pools
function profitsForWhitelistedPools() external view returns (uint256[] memory _calculatedProfits)
{
    bytes32[] memory poolIDs = poolsConfig.whitelistedPools();

    _calculatedProfits = new uint256[](poolIDs.length);
    _calculateArbitrageProfits( poolIDs, _calculatedProfits );
}

Furthermore, since Upkeep::step7 uses profitsForWhitelistedPools to distribute rewards for pools, thus the pool will get more rewards than expected since his _arbitrageProfits is never erased.

function step7() public onlySameContract
{
    uint256[] memory profitsForPools = pools.profitsForWhitelistedPools();

    bytes32[] memory poolIDs = poolsConfig.whitelistedPools();
    saltRewards.performUpkeep(poolIDs, profitsForPools );
    pools.clearProfitsForPools();
}

Tools Used

Manual

Recommended Mitigation Steps

It would be proper to add a counter for upkeep, and every time counter++ during upkeep, the previous counter won't be visited again.

Assessed type

ERC20

Redundant Function countryIsExcluded

The function countryIsExcluded only returns the excludedCountries[country], however, since this info could be retrieved directly from excludedCountries(country) as excludedCountries is public. The function defined here is redundant. The same works for Proposals::ballotForID, Proposals::lastUserVoteForBallot and Proposals::votesCastForBallot.

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