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

SwapAction.sol#transferAndSwap is still payable though direct ETH is not supported anymore. #12

Open
c4-bot-8 opened this issue Oct 18, 2024 · 2 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_07_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/main/src/proxy/SwapAction.sol#L102

Vulnerability details

Impact

SwapAction.sol#transferAndSwap supports native ETH transfer, but all native ETH swaps (balancer, pendle) are removed.

Bug Description

Note: This is based on the 2024-07 Loopfi contest code-423n4/2024-07-loopfi-findings#70 issue. This protocol team applied a fix, but the fix is incomplete.

The original fix was applied that all native ETH support is removed, and all payable modifiers for external functions are removed. However, SwapAction.sol#transferAndSwap was left out, and it is still marked payable.

    function transferAndSwap(
        address from,
        PermitParams calldata permitParams,
        SwapParams calldata swapParams
@>  ) external payable returns (uint256) {
        if (from != address(this)) {
            uint256 amount = swapParams.swapType == SwapType.EXACT_IN ? swapParams.amount : swapParams.limit;
            _transferFrom(swapParams.assetIn, from, address(this), amount, permitParams);
        }
        return swap(swapParams);
    }

Proof of Concept

N/A

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the payable modifier.

Assessed type

Payable

@c4-bot-8 c4-bot-8 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 Oct 18, 2024
c4-bot-6 added a commit that referenced this issue Oct 18, 2024
@c4-bot-11 c4-bot-11 added 🤖_07_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 18, 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 Oct 19, 2024
@amarcu amarcu added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Oct 25, 2024
@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 Nov 11, 2024
@c4-judge
Copy link

koolexcrypto changed the severity to QA (Quality Assurance)

@c4-judge
Copy link

koolexcrypto marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_07_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants