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

Denial of Service when Delegated Address Wants to Reject a Previously Confirmed Delegation. #5

Closed
c4-bot-10 opened this issue Nov 9, 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 🤖_05_group AI based duplicate group recommendation unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-11-ethena-labs/blob/main/contracts/ustb/UStbMinting.sol#L468
https://github.com/code-423n4/2024-11-ethena-labs/blob/main/contracts/ustb/UStbMinting.sol#L321

Vulnerability details

Finding impact

Denial of Service when Delegated address wants to reject a previously confirmed delegation which would allow delegation execution that delegate is not willing to carry out for the delegator as noted in Order verification in the UStbMinting contract

Description and Proof of Concept

    /// @notice assert validity of signed order
    function verifyOrder(
        Order calldata order,
        Signature calldata signature
    ) public view override returns (bytes32 taker_order_hash) {
        taker_order_hash = hashOrder(order);
        if (signature.signature_type == SignatureType.EIP712) {
            address signer = ECDSA.recover(taker_order_hash, signature.signature_bytes);
            if (
                !(signer == order.benefactor ||
>>>                   delegatedSigner[signer][order.benefactor] == DelegatedSignerStatus.ACCEPTED)
            ) {
                revert InvalidEIP712Signature();
            }  This report wants to bring to attention of protocol that
...

The code above shows how Order is verified in the UStbMinting contract, as noted in the pointer above, verification would go through if DelegatedSignerStatus is accepted. It can be noted in contract implementation that when delegated Signer is being set it takes the agreement of both the delegator and delegate in a two step process for the delegated Signer to be set in setDelegatedSigner(…) and confirmDelegatedSigner(…) function by the delegator and delegate respectively, however when delegated signer is to be removed only one party (the delegator) has the power to reverse delegation which would cause denial of service to a delegate who wants to also remove delegation.

    /// @notice The delegated address to confirm delegation
    function confirmDelegatedSigner(address _delegatedBy) external {
        if (delegatedSigner[msg.sender][_delegatedBy] != DelegatedSignerStatus.PENDING) {
            revert DelegationNotInitiated();
        }
>>>        delegatedSigner[msg.sender][_delegatedBy] = DelegatedSignerStatus.ACCEPTED;
        emit DelegatedSignerAdded(msg.sender, _delegatedBy);
    }

The code above shows how a Delegated address accepts delegation smart contracts following a two step process by both delegator and delegate from pending status to confirmation status. Then in the situation of undelegating, it can be noted at L326 that a smart contract can undelegated an address for signing. However there is no implementation in situation when a delegate wants to undelegate a contracts when it does not want to perform signing for it

Recommended mitigation steps

As provided below Ethena protocol should add the rejectDelegatedSigner to the UStbMinting.sol contract to ensure delegate can reject delegatation when necessary

+++    function rejectDelegatedSigner(address delegatedAddress) external {
+++        if (delegatedSigner[msg.sender][delegatedAddress] != DelegatedSignerStatus.ACCEPTED) {
+++            revert DelegationNotAccepted();
+++        }
+++        delegatedSigner[msg.sender][delegatedAddress] = DelegatedSignerStatus.REJECTED;
+++        emit DelegatedSignerAdded(msg.sender, delegatedAddress);
+++    }
@c4-bot-10 c4-bot-10 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 Nov 9, 2024
c4-bot-8 added a commit that referenced this issue Nov 9, 2024
@c4-bot-11 c4-bot-11 added the 🤖_05_group AI based duplicate group recommendation label Nov 11, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 12, 2024
@c4-judge
Copy link

0xEVom marked the issue as unsatisfactory:
Invalid

@0xEVom
Copy link

0xEVom commented Nov 12, 2024

It is not an issue for an address to remain delegated but inactive.

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 🤖_05_group AI based duplicate group recommendation unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants