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

No slippage applied when Withdrawing collateral from the vault and dst != collateralToken #31

Closed
howlbot-integration bot opened this issue Oct 19, 2024 · 2 comments
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 downgraded by judge Judge downgraded the risk level of this issue duplicate-10 🤖_primary AI based primary recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/PositionActionPendle.sol#L55-L84

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/PositionActionPendle.sol#L55-L84

    function _onWithdraw(
        address vault,
        address position,
        address dst,
        uint256 amount
    ) internal override returns (uint256) {
        uint256 collateralWithdrawn = ICDPVault(vault).withdraw(address(position), amount);
        address collateralToken = address(ICDPVault(vault).token());

        if (dst != collateralToken && dst != address(0)) {
            PoolActionParams memory poolActionParams = PoolActionParams({
                protocol: Protocol.PENDLE,
                minOut: 0,
                recipient: address(this),
                args: abi.encode(
                    collateralToken,
                    collateralWithdrawn,
                    dst
                )
            });

            bytes memory exitData = _delegateCall(
                address(poolAction),
                abi.encodeWithSelector(poolAction.exit.selector, poolActionParams)
            );

            collateralWithdrawn = abi.decode(exitData, (uint256));
        }
        return collateralWithdrawn;
    }

This function is used to withdraw collateral from the vault.

In the new scope there is a check to see if dst != collateralToken then a pool action param is then needed to withdraw the collateral from PENDLE, issue however is that when doing this we hardcode the slippage to 0, causing for loss of funds for users.

Impact

Loss of funds for users when withdrawing from the vault and dst != collateralToken, considering minOut has been hardcoded to 0 allowing even 0 value to be given to them post their exit from Pendle.

Recommended Mitigation Steps

Allow users pass in a slippage protected value and in the case where dst != collateralToken this value should be attached to minOut:

            PoolActionParams memory poolActionParams = PoolActionParams({
                protocol: Protocol.PENDLE,
-                minOut: 0,
+                minOut: _minOut,
                recipient: address(this),
                args: abi.encode(
                    collateralToken,
                    collateralWithdrawn,
                    dst
                )
            });

Assessed type

Context

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_primary AI based primary recommendation bug Something isn't working duplicate-10 sufficient quality report This report is of sufficient quality labels Oct 19, 2024
howlbot-integration bot added a commit that referenced this issue Oct 19, 2024
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 11, 2024
@c4-judge
Copy link

koolexcrypto marked the issue as satisfactory

@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 downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Nov 11, 2024
@c4-judge
Copy link

koolexcrypto changed the severity to 2 (Med Risk)

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 downgraded by judge Judge downgraded the risk level of this issue duplicate-10 🤖_primary AI based primary recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

1 participant