c4-bot-4 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Tokens(SALT, DAI, USDS) can be locked in the DAO contract because the contract doesn't refund remaining amount of the tokens.

Proof of Concept

If a user calls the performUpkeep function of the Upkeep contract, step3 and step4 is called and these functions also called the _formPOL function.

    // 3. Convert a default 5% of the remaining WETH to USDS/DAI Protocol Owned Liquidity.
    function step3() public onlySameContract
        {
        uint256 wethBalance = weth.balanceOf( address(this) );
        if ( wethBalance == 0 )
            return;

        // A default 5% of the remaining WETH will be swapped for USDS/DAI POL.
        uint256 amountOfWETH = wethBalance * stableConfig.percentArbitrageProfitsForStablePOL() / 100;
        _formPOL(usds, dai, amountOfWETH);
        }

    // 4. Convert a default 20% of the remaining WETH to SALT/USDS Protocol Owned Liquidity.
    function step4() public onlySameContract
        {
        uint256 wethBalance = weth.balanceOf( address(this) );
        if ( wethBalance == 0 )
            return;

        // A default 20% of the remaining WETH will be swapped for SALT/USDS POL.
        uint256 amountOfWETH = wethBalance * daoConfig.arbitrageProfitsPercentPOL() / 100;
        _formPOL(salt, usds, amountOfWETH);
        }

And then, the _formPOL function calls formPOL function of the DAO contact.

    function _formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountWETH) internal
        {
        uint256 wethAmountPerToken = amountWETH >> 1;

        // Swap WETH for the specified tokens
        uint256 amountA = pools.depositSwapWithdraw( weth, tokenA, wethAmountPerToken, 0, block.timestamp );
        uint256 amountB = pools.depositSwapWithdraw( weth, tokenB, wethAmountPerToken, 0, block.timestamp );

        // Transfer the tokens to the DAO
        tokenA.safeTransfer( address(dao), amountA );
        tokenB.safeTransfer( address(dao), amountB );

        // Have the DAO form POL
        dao.formPOL(tokenA, tokenB, amountA, amountB);
        }

    function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "DAO.formPOL is only callable from the Upkeep contract" );

        // Use zapping to form the liquidity so that all the specified tokens are used
        collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );

        emit POLFormed(tokenA, tokenB, amountA, amountB);
        }

And formPOL function calls the depositLiquidityAndIncreaseShare function of the collateralAndLiquidity contract, and then _depositLiquidityAndIncreaseShare function is called.

    function depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 deadline, bool useZapping ) external nonReentrant ensureNotExpired(deadline) returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        require( PoolUtils._poolID( tokenA, tokenB ) != collateralPoolID, "Stablecoin collateral cannot be deposited via Liquidity.depositLiquidityAndIncreaseShare" );

        return _depositLiquidityAndIncreaseShare(tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, useZapping);
        }

    function _depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        require( exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access" );

        // Transfer the specified maximum amount of tokens from the user
        tokenA.safeTransferFrom(msg.sender, address(this), maxAmountA );
        tokenB.safeTransferFrom(msg.sender, address(this), maxAmountB );

        // Balance the token amounts by swapping one to the other before adding the liquidity?
        if ( useZapping )
            (maxAmountA, maxAmountB) = _dualZapInLiquidity(tokenA, tokenB, maxAmountA, maxAmountB );

        // Approve the liquidity to add
        tokenA.approve( address(pools), maxAmountA );
        tokenB.approve( address(pools), maxAmountB );

        // Deposit the specified liquidity into the Pools contract
        // The added liquidity will be owned by this contract. (external call to Pools contract)
        bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
        (addedAmountA, addedAmountB, addedLiquidity) = pools.addLiquidity( tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]);

        // Increase the user's liquidity share by the amount of addedLiquidity.
        // Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive)
        // _increaseUserShare confirms the pool as whitelisted as well.
        _increaseUserShare( msg.sender, poolID, addedLiquidity, true );

        // If any of the user's tokens were not used, then send them back
@>      if ( addedAmountA < maxAmountA )
            tokenA.safeTransfer( msg.sender, maxAmountA - addedAmountA );

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

        emit LiquidityDeposited(msg.sender, address(tokenA), address(tokenB), addedAmountA, addedAmountB, addedLiquidity);
        }

As you can see in the _depositLiquidityAndIncreaseShare function, when the added amount of tokenA or tokenB is greater than maxAmount of them, it refund the left amount to the msg.sender. However, msg.sender is the address of the DAO contract. And the DAO contract doesn't refund the received tokens to the user again. Therefore, the tokens(SALT, USDS, DAI) would remain and be locked in the DAO contract.

Tools Used

VS Code

Recommended Mitigation Steps

Add the logic that refund the exceeded amount of tokens to the user.

Assessed type

Token-Transfer

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #721

c4-judge commented 6 months ago

Picodes marked the issue as satisfactory