jennifer37 - Lack of execution fee mechanism in AccountFacet

[sherlock-admin3]

jennifer37

Medium

Lack of execution fee mechanism in AccountFacet

Summary

When the keeper execute executeWithdraw or cancelWithdraw, no execution fee is payed for the keeper.

Vulnerability Detail

In OrderFacet and StakeFaucet, when the keepers execute increase order/ stake tokens, there will be some execution fee for the keepers. However, in AccountFacet, we lack of execution fee mechanism. Considering if gas price increases or there is not enough motivation to trigger executeWithdraw or cancelWithdraw. This will cause traders' redeem may be blocked.

    function executeWithdraw(uint256 requestId, OracleProcess.OracleParam[] calldata oracles) external override {
        RoleAccessControl.checkRole(RoleAccessControl.ROLE_KEEPER);
        Withdraw.Request memory request = Withdraw.get(requestId);
        if (request.account == address(0)) {
            revert Errors.WithdrawRequestNotExists();
        }
        OracleProcess.setOraclePrice(oracles);
        AssetsProcess.executeWithdraw(requestId, request);
        OracleProcess.clearOraclePrice();
    }

Impact

The keepers has less motivation to trigger executeWithdraw or cancelWithdraw compared with other operations. This will block the traders' collateral withdraw.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/facets/AccountFacet.sol#L48-L57

Tool used

Manual Review

Recommendation

Add execution fee mechanism for AccountFacet.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/36

[ctmotox2]

Escalate This issue should be marked as invalid because it reflects a design choice rather than a vulnerability. The decision to not include an execution fee mechanism in AccountFacet may be intentional and aligned with the overall design and upgrade strategy of the protocol. Since the contract is Diamond-upgradable, a function to withdraw fees can be introduced in the future if necessary. Therefore, this does not constitute a security vulnerability but rather a design decision.

[sherlock-admin3]

Escalate This issue should be marked as invalid because it reflects a design choice rather than a vulnerability. The decision to not include an execution fee mechanism in AccountFacet may be intentional and aligned with the overall design and upgrade strategy of the protocol. Since the contract is Diamond-upgradable, a function to withdraw fees can be introduced in the future if necessary. Therefore, this does not constitute a security vulnerability but rather a design decision.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[johnson37]

I have double confirmed with the sponsor before I submitted this finding. There is no decision to not include an execution fee mechanism. They just miss this part.

[nevillehuang]

Considering that all other executions (redemption/deposits/orders) have execution fees in place, I believe this is a valid issue if not keepers are not incentivize to pay gas to execute withdrawals for users. The loss here would be the gas fees for them (they have no benefit in following through with withdrawals)

[WangSecurity]

@johnson37 could you provide screenshots of you confirming this with the sponsor?

[johnson37]

@WangSecurity , this is one screenshot from my private thread.

[WangSecurity]

Thank you, based on the comments above and this I believe it should remain a valid issue.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Ctmotox2: rejected