sherlock-audit / 2023-06-tokemak-judging

10 stars 8 forks source link

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 1 year ago

sherlock-admin commented 1 year ago

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

sherlock-admin2 commented 1 year ago

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

JeffCX commented 1 year ago

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 commented 1 year ago

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.

Trumpero commented 1 year ago

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 commented 1 year ago

Sponsor said emergency withdrawal or pause is an unacceptable risk.

Did you read it as "acceptable" sir?

JeffCX commented 1 year ago

Some discussion is happening https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Yes, you are right

Evert0x commented 1 year ago

Planning to accept escalation and label issue as valid

JeffCX commented 1 year ago

thanks๐Ÿ‘๐Ÿ™

Evert0x commented 1 year ago

@Trumpero would you agree with high severity?

Trumpero commented 1 year ago

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

JeffCX commented 1 year ago

Agree with medium, https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/570 similar finding about external admin turn into malicious risk is marked as medium as well

Evert0x commented 1 year ago

Result: Medium Unique

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: