sherlock-admin3 commented 6 months ago

Bauchibred

high

Curve admin can drain pool via reentrancy

Summary

Curve admin can drain pool via reentrancy and rug Zivoe funds

Vulnerability Detail

From the readMe the below has been stated in regards to external admins protocol integrate with

### Q: Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?
RESTRICTED
___

Also the below has been clarified, too from https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/README.md#L87-L90

### Q: 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.
Not acceptable
___

Take a look at the series of some of the external integrations from OCY_Convex_A.sol https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCY/OCY_Convex_A.sol#L59-L69

    /// @dev Convex information.
    address public convexDeposit = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
    address public convexRewards = 0x26598e3E511ADFadefD70ab2C3475Ff741741104;
    address public convexPoolToken = 0xB30dA2376F63De30b42dC055C93fa474F31330A5;

    uint256 public convexPoolID = 106;

    /// @dev Curve information.
    address public curveBasePool = 0xDcEF968d416a41Cdac0ED8702fAC8128A64241A2;
    address public curveBasePoolToken = 0x3175Df0976dFA876431C2E9eE6Bc45b65d3473CC; /// @dev Index 1, MetaPool
    address public curveMetaPool = 0xB30dA2376F63De30b42dC055C93fa474F31330A5;      /// @dev MetaPool & Token

We can see that the metapool and token address that's been integrated is 0xB30dA2376F63De30b42dC055C93fa474F31330A5

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

Admin of curve pools can easily drain curve pools via reentrancy through the withdraw_admin_fees() function.

@external
def withdraw_admin_fees():
    # transfer coin 0 to Factory and call `convert_fees` to swap it for coin 1
    factory: address = self.factory
    coin: address = self.coins[0]
    amount: uint256 = ERC20(coin).balanceOf(self) - self.balances[0]
    if amount > 0:
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(factory, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)
        Factory(factory).convert_metapool_fees()

    # transfer coin 1 to the receiver
    coin = self.coins[1]
    amount = ERC20(coin).balanceOf(self) - self.balances[1]
    if amount > 0:
        receiver: address = Factory(factory).get_fee_receiver(self)
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(receiver, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)

The admin of the curve can set the receiver to a malicious smart contract and reenter withdraw_admin_fees for as much times to drain the pool even if the amount = ERC20(coin).balanceOf(self) - self.balances[1] is small via the raw_call() query, note that unlike all other function/ method calls in the 0xB30dA2376F63De30b42dC055C93fa474F31330A5 contract that implement the raw_call function, only the withdraw_admin_fees() has not been tagged as @nonreentrant allowing this to be possible.

Impact

Curve admins can drain pool via reentrancy thereby rugging Zivoe and it's users

Code Snippet

https://etherscan.io/address/0xB30dA2376F63De30b42dC055C93fa474F31330A5#code

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCY/OCY_Convex_A.sol#L59-L69

Tool used

Manual Review

Recommendation

Ensure that the protocol team and its users are aware of the risks of such an event and develop a contingency plan to manage it.

pseudonaut commented 6 months ago

I suppose valid, not of concern

sherlock-admin4 commented 6 months ago

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

panprog commented:

invalid, there is nothing can be done in such case, so it's just informational issue

Bauchibred commented 6 months ago

This should be valid for the scope of this contest, as already hinted in the report, the below has been mentioned in the readMe:

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/README.md#L87-L90

### Q: 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.

Not acceptable

---

NB: The above alone should be enough grounds for the validation of this issue, since emergency withdrawals are not acceptable.

Additionally, in regards to external admins protocol integrate with this was indicated https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/README.md#L40-L42

### Q: Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?

RESTRICTED

---

From Sherlock's Judging docs: https://docs.sherlock.xyz/audits/judging/judging

How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Here is a previous validated finding on the same grounds as this issue: source.

More validated issues on the grounds of Restricted Admins can be seen: here and here.

0x73696d616f commented 6 months ago

Escalate

This should be valid as the contest readme clearly indicates

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. Not acceptable

Here is the same issue and was considered valid.

Please see @Bauchibred comment for more details.

sherlock-admin3 commented 6 months ago

Escalate

This should be valid as the contest readme clearly indicates

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. Not acceptable

Here is the same issue and was considered valid.

Please see @Bauchibred comment for more details.

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.

panprog commented 6 months ago

It doesn't make sense to report any issues where nothing can be done about it. So external admin has stolen all funds and so what? USDT can go to 0 or blacklist all users or change implementation to always revert etc etc. How this can be fixed? I believe external restricted admins means they can do something for normal protocol operation (like update some settings), not for malicious actions.

0x73696d616f commented 6 months ago

This is not even my issue just trying to be helpful here, but past contests are considered to be the source of truth and several examples were shown of the same exact issue being accepted, in the same readme conditions

CergyK commented 6 months ago

I believe this reentrancy is only possible on native pools (having ETH as a coin), this is not the case of Zivoe pools IIRC

Bauchibred commented 6 months ago

It doesn't make sense to report any issues where nothing can be done about it. So external admin has stolen all funds and so what? USDT can go to 0 or blacklist all users or change implementation to always revert etc etc. How this can be fixed? I believe external restricted admins means they can do something for normal protocol operation (like update some settings), not for malicious actions.

@panprog I disagree with your reasoning above, this is not how Sherlock judging works.

I believe this reentrancy is only possible on native pools (having ETH as a coin), this is not the case of Zivoe pools IIRC

@CergyK I disagree, I'd appreciate if you could provide more info to back your claim as I can't seem to see how and why you think so. Please go through the submission again especially this part:

note that unlike all other function/ method calls in the 0xB30dA2376F63De30b42dC055C93fa474F31330A5 contract that implement the raw_call function, only the withdraw_admin_fees() has not been tagged as @nonreentrant allowing this to be possible.

Currently, the only argument you're adding to this discussion is reliant on your "belief" which imo is not valuable to the discussion.

CergyK commented 6 months ago

@CergyK I disagree, I'd appreciate if you could provide more info to back your claim as I can't seem to see how and why you think so. Please go through the submission again especially this part:
note that unlike all other function/ method calls in the [0xB30dA2376F63De30b42dC055C93fa474F31330A5](https://etherscan.io/address/0xB30dA2376F63De30b42dC055C93fa474F31330A5#code) contract that implement the raw_call function, only the withdraw_admin_fees() has not been tagged as @nonreentrant allowing this to be possible.
Currently, the only argument you're adding to this discussion is reliant on your "belief" which imo is not valuable to the discussion.

Respectfully, not seeing how it is relevant to the discussion shows you lack in depth understanding on how this vulnerability works. Let's take a look at how the withdraw_admin_fees is implemented in the accepted issue in the tokemak contest:

@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])

To handle ETH native token, developers have added a raw_call on the receiver address which is controlled by the attacker, and can trigger the reentrancy.

No such thing in the pools used by Zivoe, the implementation is as follows:

@external
def withdraw_admin_fees():
    # transfer coin 0 to Factory and call `convert_fees` to swap it for coin 1
    factory: address = self.factory
    coin: address = self.coins[0]
    amount: uint256 = ERC20(coin).balanceOf(self) - self.balances[0]
    if amount > 0:
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(factory, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)
        Factory(factory).convert_metapool_fees()

    # transfer coin 1 to the receiver
    coin = self.coins[1]
    amount = ERC20(coin).balanceOf(self) - self.balances[1]
    if amount > 0:
        receiver: address = Factory(factory).get_fee_receiver(self)
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(receiver, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)

There is no raw_call on an attacker controlled address which can trigger the reentrancy

marchev commented 6 months ago

Agree with @CergyK. The raw_call in the Zivoe pool is simply a low-level call to the coin transfer() function. Thus, at no point the control is passed to a malicious receiver (they simply receive the coin), thus they cannot re-enter. IMHO this issue should remain invalid.

Bauchibred commented 6 months ago

@CergyK I disagree, I'd appreciate if you could provide more info to back your claim as I can't seem to see how and why you think so. Please go through the submission again especially this part:
note that unlike all other function/ method calls in the [0xB30dA2376F63De30b42dC055C93fa474F31330A5](https://etherscan.io/address/0xB30dA2376F63De30b42dC055C93fa474F31330A5#code) contract that implement the raw_call function, only the withdraw_admin_fees() has not been tagged as @nonreentrant allowing this to be possible.
Currently, the only argument you're adding to this discussion is reliant on your "belief" which imo is not valuable to the discussion.
Respectfully, not seeing how it is relevant to the discussion shows you lack in depth understanding on how this vulnerability works. Let's take a look at how the withdraw_admin_fees is implemented in the accepted issue in the tokemak contest:
@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])
To handle ETH native token, developers have added a raw_call on the receiver address which is controlled by the attacker, and can trigger the reentrancy.

No such thing in the pools used by Zivoe, the implementation is as follows:
@external
def withdraw_admin_fees():
    # transfer coin 0 to Factory and call `convert_fees` to swap it for coin 1
    factory: address = self.factory
    coin: address = self.coins[0]
    amount: uint256 = ERC20(coin).balanceOf(self) - self.balances[0]
    if amount > 0:
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(factory, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)
        Factory(factory).convert_metapool_fees()

    # transfer coin 1 to the receiver
    coin = self.coins[1]
    amount = ERC20(coin).balanceOf(self) - self.balances[1]
    if amount > 0:
        receiver: address = Factory(factory).get_fee_receiver(self)
        response: Bytes[32] = raw_call(
            coin,
            _abi_encode(receiver, amount, method_id=method_id("transfer(address,uint256)")),
            max_outsize=32,
        )
        if len(response) > 0:
            assert convert(response, bool)
There is no raw_call on an attacker controlled address which can trigger the reentrancy

Hi @CergyK appreciate you providing more information about the differences in the implementation for withdraw_admin_fees. I now see how the call's context in this case never gets controlled by an attacker address while the transfer is being processed, so I concur that this issue is indeed invalid.