Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoolAction.sol#_balancerJoin does not support native ETH as input token. #69

Closed
c4-bot-3 opened this issue Aug 15, 2024 · 4 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-70 🤖_primary AI based primary recommendation 🤖_121_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality

Comments

@c4-bot-3
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/proxy/PoolAction.sol#L142-L152

Vulnerability details

Impact

PoolAction.sol#_balancerJoin does not support native ETH as input token.

Bug Description

PoolAction is used to join a Balancer or Pendle pool. Both Balancer and Pendle accepts native ETH as input token.

We can check that the PoolAction contract also supports passing native ETH as input token, because 1) join() function, which serves as the entry function, is payable; 2) _pendleJoin() passes msg.value along when calling pendleRouter.addLiquiditySingleToken().

However, the issue is that when joining balancer pool, the msg.value is not passed along.

    function join(PoolActionParams memory poolActionParams) public payable {
        if (poolActionParams.protocol == Protocol.BALANCER) {
            _balancerJoin(poolActionParams);
        } else if(poolActionParams.protocol == Protocol.PENDLE) {
            _pendleJoin(poolActionParams);
        } else {
            revert PoolAction__join_unsupportedProtocol();
        }
    }

     function _balancerJoin(PoolActionParams memory poolActionParams) internal {
        (bytes32 poolId, address[] memory assets, uint256[] memory assetsIn, uint256[] memory maxAmountsIn) = abi
            .decode(poolActionParams.args, (bytes32, address[], uint256[], uint256[]));

        for (uint256 i = 0; i < assets.length; ) {
            if (maxAmountsIn[i] != 0) {
                IERC20(assets[i]).forceApprove(address(balancerVault), maxAmountsIn[i]);
            }

            unchecked {
                ++i;
            }
        }

        // @auditnote: BUG, msg.value is not passed.
>       balancerVault.joinPool(
            poolId,
            address(this),
            poolActionParams.recipient,
            JoinPoolRequest({
                assets: assets,
                maxAmountsIn: maxAmountsIn,
                userData: abi.encode(JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT, assetsIn, poolActionParams.minOut),
                fromInternalBalance: false
            })
        );
    }

    function _pendleJoin(PoolActionParams memory poolActionParams) internal {
        (
            address market,
            ApproxParams memory guessPtReceivedFromSy,
            TokenInput memory input,
            LimitOrderData memory limit
        ) = abi.decode(poolActionParams.args, (address, ApproxParams, TokenInput , LimitOrderData));
        
        
        if (input.tokenIn != address(0)) {
                IERC20(input.tokenIn ).forceApprove(address(pendleRouter),input.netTokenIn);
            }

        pendleRouter.addLiquiditySingleToken{value: msg.value}(poolActionParams.recipient, market, poolActionParams.minOut, guessPtReceivedFromSy, input, limit);
    }

Proof of Concept

N/A

Tools Used

Manual Review

Recommended Mitigation Steps

-       balancerVault.joinPool(
+       balancerVault.joinPool{value: msg.value}(

Assessed type

Other

@c4-bot-3 c4-bot-3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Aug 15, 2024
c4-bot-4 added a commit that referenced this issue Aug 15, 2024
@c4-bot-12 c4-bot-12 added 🤖_121_group 🤖_121_group AI based duplicate group recommendation and removed 🤖_121_group labels Aug 15, 2024
@c4-bot-11 c4-bot-11 added the 🤖_primary AI based primary recommendation label Aug 15, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Aug 20, 2024
@0xtj24 0xtj24 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Sep 19, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 1, 2024

koolexcrypto changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 1, 2024

This previously downgraded issue has been upgraded by koolexcrypto

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Oct 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 1, 2024

koolexcrypto marked the issue as duplicate of #70

@c4-judge c4-judge closed this as completed Oct 1, 2024
@c4-judge c4-judge added duplicate-70 satisfactory satisfies C4 submission criteria; eligible for awards and removed primary issue Highest quality submission among a set of duplicates labels Oct 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 1, 2024

koolexcrypto marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-70 🤖_primary AI based primary recommendation 🤖_121_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants