Skip to content
This repository has been archived by the owner on Mar 3, 2024. It is now read-only.

ctf_sec - curve admin can drain pool via reentrancy (equal to execute emergency withdraw and rug tokenmak fund by third party) #862

Open
sherlock-admin opened this issue Aug 30, 2023 · 17 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Aug 30, 2023

ctf_sec

high

curve admin can drain pool via reentrancy (equal to execute emergency withdraw and rug tokenmak fund by third party)

Summary

curve admin can drain pool via reentrancy (equal to execute emergency withdraw and rug tokenmak fund)

Vulnerability Detail

A few curve liquidity is pool is well in-scope:

Curve Pools

Curve stETH/ETH: 0x06325440D014e39736583c165C2963BA99fAf14E
Curve stETH/ETH ng: 0x21E27a5E5513D6e65C4f830167390997aA84843a
Curve stETH/ETH concentrated: 0x828b154032950C8ff7CF8085D841723Db2696056
Curve stETH/frxETH: 0x4d9f9D15101EEC665F77210cB999639f760F831E
Curve rETH/ETH: 0x6c38cE8984a890F5e46e6dF6117C26b3F1EcfC9C
Curve rETH/wstETH: 0x447Ddd4960d9fdBF6af9a790560d0AF76795CB08
Curve rETH/frxETH: 0xbA6c373992AD8ec1f7520E5878E5540Eb36DeBf1
Curve cbETH/ETH: 0x5b6C539b224014A09B3388e51CaAA8e354c959C8
Curve cbETH/frxETH: 0x548E063CE6F3BaC31457E4f5b4e2345286274257
Curve frxETH/ETH: 0xf43211935C781D5ca1a41d2041F397B8A7366C7A
Curve swETH/frxETH: 0xe49AdDc2D1A131c6b8145F0EBa1C946B7198e0BA

one of the pool is 0x21E27a5E5513D6e65C4f830167390997aA84843a

https://etherscan.io/address/0x21E27a5E5513D6e65C4f830167390997aA84843a#code#L1121

Admin of curve pools can easily drain curve pools via reentrancy or via the withdraw_admin_fees function.

@external
def withdraw_admin_fees():
    receiver: address = Factory(self.factory).get_fee_receiver(self)

    amount: uint256 = self.admin_balances[0]
    if amount != 0:
        raw_call(receiver, b"", value=amount)

    amount = self.admin_balances[1]
    if amount != 0:
        assert ERC20(self.coins[1]).transfer(receiver, amount, default_return_value=True)

    self.admin_balances = empty(uint256[N_COINS])

if admin of the curve can set a receiver to a malicious smart contract and reenter withdraw_admin_fees a 1000 times to drain the pool even the admin_balances is small

the line of code

raw_call(receiver, b"", value=amount)

trigger the reentrancy

This is a problem because as stated by the tokemak team:

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Pausing or emergency withdrawals are not acceptable for Tokemak.

As you can see above, pausing or emergency withdrawals are not acceptable, and this is possible for cuve pools so this is a valid issue according to the protocol and according to the read me

Impact

curve admins can drain pool via reentrancy

Code Snippet

https://etherscan.io/address/0x21E27a5E5513D6e65C4f830167390997aA84843a#code#L1121

Tool used

Manual Review

Recommendation

N/A

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Sep 11, 2023
@sherlock-admin2
Copy link
Contributor

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, as the submission stated: "Admin of curve pools can easily drain curve pools via reentrancy", so no vulnerability for tokemak here

@sherlock-admin2 sherlock-admin2 changed the title Helpful Amber Llama - curve admin can drain pool via reentrancy (equal to execute emergency withdraw and rug tokenmak fund by third party) ctf_sec - curve admin can drain pool via reentrancy (equal to execute emergency withdraw and rug tokenmak fund by third party) Oct 3, 2023
@sherlock-admin2 sherlock-admin2 added the Non-Reward This issue will not receive a payout label Oct 3, 2023
@JeffCX
Copy link

JeffCX commented Oct 3, 2023

Escalate

as the protocol docs mentioned

https://audits.sherlock.xyz/contests/101

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Pausing or emergency withdrawals are not acceptable for Tokemak.

in the issue got exploit in this report, user from tokenmak lose fund as well

@sherlock-admin2
Copy link
Contributor

Escalate

as the protocol docs mentioned

https://audits.sherlock.xyz/contests/101

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Pausing or emergency withdrawals are not acceptable for Tokemak.

in the issue got exploit in this report, user from tokenmak lose fund as well

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Oct 3, 2023
@Trumpero
Copy link
Collaborator

Trumpero commented Oct 7, 2023

Hi @JeffCX, based on this comment of sponsors in the contest channel, I think this issue should be marked as low/invalid:
https://discord.com/channels/812037309376495636/1130514263522410506/1143588977962647582
Screenshot 2023-10-07 at 15 01 02

@JeffCX
Copy link

JeffCX commented Oct 7, 2023

Sponsor said emergency withdrawal or pause is an unacceptable risk.

Did you read it as "acceptable" sir?

@JeffCX
Copy link

JeffCX commented Oct 7, 2023

Some discussion is happening #899

but this is a separate external integration risk than the balancer one that can impact tokemak user :) and don't think this is a known issue

@Trumpero
Copy link
Collaborator

Hello @JeffCX,

Upon further consideration of this matter, I find it to be valid. The potential for the curve admin to exploit the reentrancy-attack and drain the curve pool could have a direct impact on the Tokemak protocol.

I suggest that you review this issue as well, @codenutt.

@JeffCX
Copy link

JeffCX commented Oct 13, 2023

Hello @JeffCX,

Upon further consideration of this matter, I find it to be valid. The potential for the curve admin to exploit the reentrancy-attack and drain the curve pool could have a direct impact on the Tokemak protocol.

I suggest that you review this issue as well, @codenutt.

Thank you very much! 😄🎉!!

@codenutt
Copy link

Thanks @Trumpero / @JeffCX! Just to confirm, this is an issue with some Curve pools just in general, correct? Not necessarily with a particular interaction we have with them.

@Trumpero
Copy link
Collaborator

Yes, you are right

@Evert0x
Copy link
Contributor

Evert0x commented Oct 23, 2023

Planning to accept escalation and label issue as valid

@JeffCX
Copy link

JeffCX commented Oct 23, 2023

thanks👍🙏

@Evert0x
Copy link
Contributor

Evert0x commented Oct 26, 2023

@Trumpero would you agree with high severity?

@Trumpero
Copy link
Collaborator

No I think it should be medium since it assume the curve admin become malicious

@JeffCX
Copy link

JeffCX commented Oct 26, 2023

Agree with medium, #570 similar finding about external admin turn into malicious risk is marked as medium as well

@Evert0x Evert0x added the Medium A valid Medium severity issue label Oct 26, 2023
@sherlock-admin sherlock-admin added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Oct 26, 2023
@Evert0x Evert0x reopened this Oct 26, 2023
@Evert0x
Copy link
Contributor

Evert0x commented Oct 26, 2023

Result:
Medium
Unique

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Oct 26, 2023
@sherlock-admin sherlock-admin added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 26, 2023
@sherlock-admin2
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin2 sherlock-admin2 removed the Excluded Excluded by the judge without consulting the protocol or the senior label Oct 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

6 participants