hash - Balancer vault reentrancy is not checked when interacting with BLVaultManager

[sherlock-admin]

hash

high

Balancer vault reentrancy is not checked when interacting with BLVaultManager

Summary

Lack of reentrancy check when interacting with BLVaultManager allows for manipulation of collateralized ohm value

Vulnerability Detail

Balancer vaults have a read only reentrancy vulnerability which allows to alter the (lp token / reserve balances) ratio. This issue is known by the team and is handled in many places by checking for reentrancy before querying. But the check is not implemented when calling the getPoolOhmShare of a BLVaultManager. The getPoolOhmShare function queries the balancer vault and pool in order to calculate the value of a share without using any reentrancy checks and is affected by the vulnerability.

    function getCollateralizedOhm() external view override returns (uint256) {
        uint256 len = vaultManagers.length;
        uint256 total;
        for (uint256 i; i < len; ) {
            // @audit no-reentrancy checks
            total += vaultManagers[i].getPoolOhmShare();

getPoolOhmShare function of BLVaultManagerLido.sol https://vscode.blockscan.com/ethereum/0xcb22361599c259dfe92eb858232f94e41a0685cf

    function getPoolOhmShare() public view override returns (uint256) {

        IVault vault = IVault(balancerData.vault);
        IBasePool pool = IBasePool(balancerData.liquidityPool);

        uint256 poolTotalSupply = pool.totalSupply();

        (, uint256[] memory balances_, ) = vault.getPoolTokens(pool.getPoolId());

        if (poolTotalSupply == 0) return 0;
        else return (balances_[0] * totalLp) / poolTotalSupply;
    }

Impact

The collateralized ohm supply value can be manipulated by an attacker. The wider system relies on the supply calculation to be correct in order to perform actions of economical impact

https://discord.com/channels/812037309376495636/1184355501258047488/1184397904551628831
it will be determined to get backing
so it will have an economical impact, as we could be exchanging ohm for treasury assets at a wrong price

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/SPPLY/submodules/BLVaultSupply.sol#L86-L91

Tool used

Manual Review

Recommendation

Check balancer vault for reentrancy

[0xJem]

This is actually a valid issue

[nevillehuang]

@0xJem, While this seems to be accurate, the watson didn't really elaborate on how the attack can be performed, which made me invalidate this issue in the first place due to unclear attack path. To me this issue is simply stating it is possible but doesn't really explain how it is possible specific to the protocol context

This would falls under this sherlock rule. Read only reentrancy is a very complex attack, so its not clear to me based on watsons description.

In case of non-obvious issues with complex vulnerabilities/attack paths, Watson must submit a valid POC for the issue to be considered valid and rewarded

[0xJem]

Sure, I'm just stating that it is a fair concern, and we've added re-entrancy checks in other locations to be sure, so wanted to be consistent in adding it here.

[Czar102]

Based on this submission, I don't think the Watson managed to prove that there is a vulnerability there. Encouraging the sponsor to apply the fix, though we can't reward such a speculative submission. Hence, agree with the Lead Judge to consider this issue an informational one.

[10xhash]

The reason I didn't provide a detailed explanation is that this vulnerability is clearly known by the team since reentrancy check is used in all other places with supporting comment. eg: https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L227-L229 Hence the exploit-ability seemed obvious if the Balancer vault could get reentered.

To view the effect please consider the POC I have attached below : https://gist.github.com/10xhash/86f28f079ab7885dc92d4be0a05c6e23

[0xJem]

https://github.com/OlympusDAO/bophades/pull/281

[0xllill]

Escalate

I think it should be valid due to how sponsor handled reentrancy within the protocol and how he forgot to add a check there Moreover gist added seems enough to provide a POC

[sherlock-admin2]

Escalate

I think it should be valid due to how sponsor handled reentrancy within the protocol and how he forgot to add a check there Moreover gist added seems enough to provide a POC

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.

[nevillehuang]

Agree that this is a valid issue, but I think the PoC should have been added to the original issue at the point of the contest since it is not the judges responsibility to validate issues for watsons (I believe a 21 day contest period is sufficient enough to do so).

Given I have hundreds of issues to go through, I cannot confirm the validity of this report by the information provided in the report alone, so I still believe it should be low severity based on the above sherlock rules I highlighted, agreed upon by the head of judging.

[10xhash]

If the balancer pool reentrancy vulnerability is known ie. reentering balancer can allow manipulation of lp token supply / token balances ratio, the issue would be obvious rather than a complex one since all that getPoolOhmShare function does is calculate this ratio.

Due to the usage of reentrancy lib for Balancer throughout the contract except at the mentioned location and the readme stating Where possible, SPPLY submodules will check for re-entrancy in the source (e.g. liquidity pool) it gave me the idea that the vulnerability is indeed known for the team. From this POV the vulnerability is an obvious one rather than complex.

Hence the report was written under this assumption. I believe the report still contains all the necessary information to validate the vulnerability ie. link to the detailed external vulnerability from a credible source, the effect of the external vulnerability allows to alter the (lp token / reserve balances) ratio, the problem in the current context getPoolOhmShare function queries the balancer vault and pool in order to calculate the value of a share, But the check is not implemented when calling the getPoolOhmShare of a BLVaultManager and the code segments of getCollateralizedOhm and getPoolOhmShare which are affected due to this.

I realize that from the POV of a judge the issue might not be obvious. But in such a scenario shouldn't the discussions during the escalation period be considered?

[nevillehuang]

@10xhash While I understand your point of view, to me, the vulnerability is known but not sufficiently proven, so I stand by my previous comment. I cannot confirm the possibility of reentrancy unless I had your PoC provided. I believe escalation period is to handle misjudged issues based on impact and information provided in the original submission, not for watsons to supplement their findings when they already had a chance during the contest phase.

I believe no further discussions is required here, we can let head of judging decide and I will respect any decision.

[IAm0x52]

Fix looks good. ensureNotInVaultContext is now called to prevent reentrancy

[Czar102]

I stand by my previous comment. I'd like to note that the impact section is not noting any concrete evidence, making the judgment justified besides the technical lack of communication.

The report must not only be understandable by the sponsor, but also by Judges.

[10xhash]

The code-base in scope is acting like a feed for supply values which is why I have mentioned the impact as above:

The collateralized ohm supply value can be manipulated by an attacker
The wider system relies on the supply calculation to be correct in order to perform actions of economical impact

https://discord.com/channels/812037309376495636/1184355501258047488/1184397904551628831
it will be determined to get backing
so it will have an economical impact, as we could be exchanging ohm for treasury assets at a wrong price

A concrete description of the impact apart from this higher level would have required the knowledge of out of scope contracts.

I agree that I could've made the report more elaborative but I am not clear on how much should be elaborated for this type of (known external integration) issues hence why I cited a credible source for the external issue and briefed the impact in the current code-base.

[nevillehuang]

@10xhash I take responsibility partially for the misjudgement, though the fact that I had to do additional research to prove validity certainly didn’t help, given based on the limited description of the issue, I couldn’t determine validity.

I believe with the new “request poc” feature, this type of instances of misjudgement will be prevented by me in the future.

[Czar102]

Result: Invalid Unique

This is a really good find, but we can't reward such an unsatisfactory report. The issue behind a hint displayed in the submission is of High severity. At some point, we need to cut off the rewards, otherwise we would be accepting arbitrarily bad reports.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nasri136: rejected