juaan - Malicious Public Vault Owner can bypass `validateRebalance()`, to sandwich the rebalance for profit

[sherlock-admin3]

juaan

high

Malicious Public Vault Owner can bypass validateRebalance(), to sandwich the rebalance for profit

Summary

The contest README states:

Public vault owner is RESTRICTED

Hence their actions are not trusted.

Vulnerability Detail

In ArrakisStandardManager.rebalance(), it uses module.validateRebalance() before and after the rebalance, in order to ensure that the AMM's spot price has not been significantly manipulated before and after the rebalance as this would allow for sandwich attacks.

// check if the underlying protocol price has not been
// manipulated during rebalance.
// that can indicate a sandwich attack.
module.validateRebalance(info.oracle, info.maxDeviation);

However, via a 2-day timelock, a malicious public vault owner can choose the oracle address that is used (by calling updateVaultInfo()), and can set it to a malicious oracle which simply returns the AMM spot price.

Example malicious oracle:

contract FakeOracle {

    HOT alm;

    constructor(address _alm) {
        alm = HOT(_alm);
    }
   // Simply returns the AMM price so that validateRebalance() is useless
    function getPrice0() external view returns (uint256 price0) {
        uint256 sqrtSpotPriceX96;
        (sqrtSpotPriceX96,,) = alm.getAMMState();

        if (sqrtSpotPriceX96 <= type(uint128).max) {
            price0 = FullMath.mulDiv(
                sqrtSpotPriceX96 * sqrtSpotPriceX96,
                10 ** 6,
                2 ** 192
            );
        } else {
            price0 = FullMath.mulDiv(
                FullMath.mulDiv(
                    sqrtSpotPriceX96, sqrtSpotPriceX96, 1 << 64
                ),
                10 ** 6,
                1 << 128
            );
        }
    }
}

Then when the price deviation is calculated in validateRebalance(), it will be 0, so won't revert even if the actual AMM price has been manipulated.

This then allows the malicious public vault owner to sandwich the rebalance and extract value from the LPs.

Impact

Malicious public vault owner can steal funds from LPs by changing oracle parameter in vaultInfo.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisStandardManager.sol#L386

Tool used

Manual Review

Recommendation

Whitelist oracles that can be used

[Gevarist]

Hi, public vault owner is restricted with this timelock contract. We are assuming that these 2 days will give enough time for user to withdraw their fund if any malicious action is sheduled inside the timelock. We don't consider this finding as valid.

[IWildSniperI]

quoting this from sherlock rules

Watsons are expected to keep up to date with the contest's Discord channel as important announcements impacting judging may arise. They should keep in mind that any message the Sponsor sends will be considered a source of truth.

[cu5t0mPeo]

escalate As described by Watsons and Sponsor, it should be low.

[sherlock-admin3]

escalate As described by Watsons and Sponsor, it should be low.

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.

[0xjuaan]

i agree this (and the dupes) can be invalidated, i submitted it just in case it was accepted for others

[iamandreiski]

Thanks everyone for the inputs, just to add a couple of arguments here objecting to the above-mentioned case. According to the Sherlock's rule version mentioned in this contest, commit: 4c718d4

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

Using discord's messages as an argument that this should be invalid directly objects the Sherlock's rules and according to the Contest README, the Public vault owner is RESTRICTED.

Now the issue explained above, coupled with the other issues mentioned as duplicates to this one, should keep one thing in common, the Timelock is deployed in a way that there's no way to either cancel any of the public vault owner's actions OR overwrite them, change admins, etc. (Thoroughly explained in issue #34 )

Making the assumption that two days are enough for every user to withdraw liquidity, but having no existing way to cancel any malicious proposal, action, etc. during those two days is a cause for concern, in my opinion. Now to further continue with the cause-effect relationship here, here are facts mentioned in the contest README:

• Public vault owner is RESTRICTED

Here is more information part of the contest documentation:

• These registries manage whitelisted module beacons, which offer the assurance of being non-upgradable or only upgradable under the strict governance of the Arrakis Timelock.
• A slightly modified generic timelock contract which ensures the timelock cannot transfer ownership of the vault away from the timelock itself. A new instance of this contract is employed for each public Arrakis vault, ensuring that security parameters cannot be hastily reconfigured by a compromised vault owner to extract value from the vault.

Finally, a discord message from the Sponsor:

• "Public vault owner is restricted due to timelock contract wrapping ownership. But if the owner can still harm users somehow, I suggest to submit the finding."

Now, taking all of the above into consideration, the hierarchy of truth, and the sponsor's comments, I don't think that anywhere it's mentioned (including known/accepted issues) that the Timelock is only in place so that users can have 2 days to withdraw funds in case of malicious actions, I believe all of the information mentioned above pertains towards having a way to cancel proposals/override them during those 2 days.

This being said, if the only safeguard against a malicious public vault owner is encouraging users to withdraw funds within two days, I believe the issues mentioned here should be valid.

[WangSecurity]

Now, taking all of the above into consideration, the hierarchy of truth, and the sponsor's comments, I don't think that anywhere it's mentioned (including known/accepted issues) that the Timelock is only in place so that users can have 2 days to withdraw funds in case of malicious actions, I believe all of the information mentioned above pertains towards having a way to cancel proposals/override them during those 2 days.

Is it anywhere clearly explained what is the purpose of these 2 days, or it's assumptions derived from difference sources of truth?

[iamandreiski]

Now, taking all of the above into consideration, the hierarchy of truth, and the sponsor's comments, I don't think that anywhere it's mentioned (including known/accepted issues) that the Timelock is only in place so that users can have 2 days to withdraw funds in case of malicious actions, I believe all of the information mentioned above pertains towards having a way to cancel proposals/override them during those 2 days.

Is it anywhere clearly explained what is the purpose of these 2 days, or it's assumptions derived from difference sources of truth?

Thanks for the question, I don't think so. The information which matches across the different sources of truth is that the owner is timelocked and that its actions are Restricted (that's why its timelocked). - That is a fact across all sources of information.

There's no information anywhere stating that the purpose of the timelock is to have a head start of 2 days so that if any malicious actions do happen users should withdraw. - This was only provided as information by the sponsor in this GitHub thread after judging was completed.

From the limited information provided, the assumption is that if a malicious action is "scheduled" that there is a way for it to be cancelled.

Since there's no way for it to be cancelled due to the way that the Timelock is deployed #34 , the hierarchy of events which will occur is Restricted Owner schedules a malicious action -> due to no way of that action being cancelled or overridden, it will occur/take place in 2 days nevertheless -> All users have two days to withdraw their funds -> Users which have failed to withdraw funds within the 48 hour period will be prone to fund loss.

The final logic as to why the issue should be valid is that taking in consideration that the owner is restricted as stated in the contest Readme (primary source of truth) and that there's no actual way to override/cancel the actions of the restricted role as they will inevitably take place and maliciously harm the depositors.

[cu5t0mPeo]

• A slightly modified generic timelock contract which ensures the timelock cannot transfer ownership of the vault away from the timelock itself. A new instance of this contract is employed for each public Arrakis vault, ensuring that security parameters cannot be hastily reconfigured by a compromised vault owner to extract value from the vault.

For users who have not adjusted their positions within two days, it should be considered their own fault.

[iamandreiski]

• A slightly modified generic timelock contract which ensures the timelock cannot transfer ownership of the vault away from the timelock itself. A new instance of this contract is employed for each public Arrakis vault, ensuring that security parameters cannot be hastily reconfigured by a compromised vault owner to extract value from the vault.

For users who have not adjusted their positions within two days, it should be considered their own fault.

Thanks for the feedback. With all due respect, the quoted point has nothing to do with users accepting fault if they haven't acted within two days, that assumption can't be made as that information was never provided in any form/documentation during the course of the audit, so in regards to your comment, the original assumption mentioned in my latest comment still stands.

From the quoted point which states that "security parameters cannot be hastily reconfigured" refers to this part which can be seen in the Timelock.sol contract:

  function updateDelay(uint256) external pure override {
        revert NotImplemented();
    }

But since the "owner" of the Timelock is the Factory contract, and the Factory contract contains no logic to interfere in any of the Timelock processes, such as cancel transactions, update admin, etc. -> this changes nothing.

There were no accepted risks or known issues which state that it's the users' obligation to withdraw funds within 2 days if owner is compromised. Thanks again!

[cu5t0mPeo]

Sorry, I quoted the wrong viewpoint.

The sponsor previously confirmed that issues that require a malicious timelocked tx to execute without anybody realizing are invalid. They mentioned in Discord suggest to submit the finding but did not acknowledge that the issue is necessarily valid. The sponsor's point is: if the risk exceeds expectations, it can be considered valid. For example, if the owner of a public vault can harm the entire protocol after two days, even if there are two days to prevent the error, it will still have an impact.

Moreover, the design of the timelock is intended to mitigate the lack of trust in the owner of the public vault. Therefore, this should be seen as the user's own fault, which is entirely avoidable, rather than unavoidable.

[iamandreiski]

Sorry, I quoted the wrong viewpoint.

The sponsor previously confirmed that issues that require a malicious timelocked tx to execute without anybody realizing are invalid. They mentioned in Discord suggest to submit the finding but did not acknowledge that the issue is necessarily valid. The sponsor's point is: if the risk exceeds expectations, it can be considered valid. For example, if the owner of a public vault can harm the entire protocol after two days, even if there are two days to prevent the error, it will still have an impact.

Moreover, the design of the timelock is intended to mitigate the lack of trust in the owner of the public vault. Therefore, this should be seen as the user's own fault, which is entirely avoidable, rather than unavoidable.

Thanks again for the reply, good points :) Although, a couple of things to be added,

The arguments made by the sponsor that you've quoted are from Discord, which according to the judging rules from this contest rule commit:

Discord messages or DM screenshots are not considered sources of truth while judging an issue/escalation especially if they are conflicting with the contest README.

Since the contest README outlines that the vault owner is RESTRICTED, this should trump all other things, especially since there's nothing in the README, even in the documentation outlining the "known issue / accepted risk" of the above-mentioned cases of users acknowledging / accepting the risk that in case of a malicious action by the owner, they'll have only 2 days to withdraw.

Second to this is how the Timelock is deployed. Since no further information is provided besides that the Timelock was slightly altered so that the delay can't be updated, it's logical to assume that according to official OZ documentation: the sole purpose of the Timelock is for the deployer of the contract to have administrative control over everything (which is not the case here), for there to be roles for different kind of actions, and administrative control and oversight in case of malicious actions (for which none are present here).

Coming back to your points made, its only an assumption of what the sponsor might have thought with the encouragement of the submissions to be made in case of a malicious owner and even then those are based upon Discord messages. The design of the Timelock actually disregards the standard practice suggested by OpenZeppelin as mentioned in the above-mentioned link and there are no accepted risks or user warnings.

Thanks!

[Gevarist]

I confirm @cu5t0mPeo point. Public vault owner is restricted by the timelock, it's not a trustless system.

[cu5t0mPeo]

Hi, there's also point 10:

"Watsons are expected to keep up to date with the contest's Discord channel as important announcements impacting judging may arise. They should keep in mind that any message the Sponsor sends will be considered a source of truth."

https://docs.sherlock.xyz/audits/judging/judging#ii.-criteria-for-issue-severity:~:text=Watsons%20are%20expected%20to%20keep%20up%20to%20date%20with%20the%20contest%27s%20Discord%20channel%20as%20important%20announcements%20impacting%20judging%20may%20arise.%20They%20should%20keep%20in%20mind%20that%20any%20message%20the%20Sponsor%20sends%20will%20be%20considered%20a%20source%20of%20truth.

[WangSecurity]

@cu5t0mPeo not about the issue, just advice for the future. Each contest has its own hash of the rules (the rules that were active when the contest started). You've sent the link to the new rules, while this contest uses this version.

@iamandreiski

From the limited information provided, the assumption is that if a malicious action is "scheduled" that there is a way for it to be cancelled.

As I see you say yourself that your assumption is also not based on the explicit information, but an assumption from the information provided. As well as the @cu5t0mPeo 's point.

The owner of the public vault is restricted, hence, it's expected that they can perform malicious actions. Additionally, it's still a fact that all the users have 2 days until the change goes live to withdraw their funds and not suffer any loss. In that case, I believe it's the users' fault if they see there will be a change in the protocol causing them to lose funds and decide to keep the funds inside the contract.

Planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cu5t0mPeo: accepted