0x52 - CustomSupply contains no methodology to set _protocolOwnedTreasuryOhm after it has been set

[sherlock-admin]

0x52

medium

CustomSupply contains no methodology to set _protocolOwnedTreasuryOhm after it has been set

Summary

CustomSupply contains the storage variable _protocolOwnedTreasuryOhm which is used by OlympusSupply when determining keep protocol metrics. It can be set in the constructor but there is no way to change the supply after. In the event that there is a large shift in Ohm allocation to or from _protocolOwnedTreasuryOhm, key supply metrics will return incorrectly which can lead to the RBS deploying incorrectly.

Vulnerability Detail

CustomSupply.sol#L46-L64

constructor(
    Module parent_,
    uint256 collateralizedOhm_,
    uint256 protocolOwnedBorrowableOhm_,
    uint256 protocolOwnedLiquidityOhm_,
    uint256 protocolOwnedTreasuryOhm_,
    address source_
) Submodule(parent_) {
    _collateralizedOhm = collateralizedOhm_;
    _protocolOwnedBorrowableOhm = protocolOwnedBorrowableOhm_;
    _protocolOwnedLiquidityOhm = protocolOwnedLiquidityOhm_;
    _protocolOwnedTreasuryOhm = protocolOwnedTreasuryOhm_;
    _source = source_;

    emit CollateralizedValueUpdated(collateralizedOhm_);
    emit ProtocolOwnedBorrowableValueUpdated(protocolOwnedBorrowableOhm_);
    emit ProtocolOwnedLiquidityValueUpdated(protocolOwnedLiquidityOhm_);
    emit SourceValueUpdated(source_);
}

Here we see _protocolOwnedTreasuryOhm set in the constructor. However no function exists to change it after it is set in the constructor, like exists for other key metrics.

Impact

The _protocolOwnedTreasuryOhm metric can become heavily skewed if tokens are moved to or from a CustomSupply submodule to a location that is automatically accounted.

Code Snippet

CustomSupply.sol#L111-L135

Tool used

Manual Review

Recommendation

Add a setProtocolOwnedBorrowableOhm method to CustomSupply

[0xJem]

low severity admin-only/permissioned function, no impact on funds

[nevillehuang]

Hi @0xJem wouldnt this force a migration if the _protocolOwnedTreasuryOhm is intended to be changed?

[0xJem]

What kind of migration are you referring to?

All of the other variables (e.g. protocol owned liquidity) can be set after deployment. This just makes it consistent across all of the variables.

[nevillehuang]

@0xJem Since you confirmed the issue, it means that there is a possibility that _protocolOwnedTreasuryOhm changes in the future. Is there any possibility that it will affect any existing logic like for example adding categories for OlympusSupply here.

@0x52 Could you provide more details on why you think this should be a possible medium severity on grounds of breaking core contract functionality/loss of funds?

[0xJem]

It could change in the future, but the other values on a contract inheriting from CustomSupply could also be changed. So why is this being treated any differently?

I don't see how it would affect categories - it would affect the supply and metric calculations, but that's expected.

Also, these set* functions are permissioned (onlyParent), so would only be changed through a policy executed by an admin.

[nevillehuang]

@0xJem If there is no explicit setter function to change this variable, wouldn't this impact the supply and metric calculations like you mentioned and thus necessitate a need to redeploy if not this calculations will be permanently affected?

[0xJem]

Yes a re-deploy would be needed without the addition of the setter. But don't forget that this is an abstract contract and won't be deployed directly. The contracts that inherit from this (SiloSupply and BrickedSupply, for example) are the ones that would be affected.

[nevillehuang]

Unlike #151 and #158, since technically this issue could be resolved with a redeployment without a direct fix to the contract code logic, I think I agree with sponsor that this is low severity since core contract functionality is not broken, so closing this issue.

[0xJem]

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

[IAm0x52]

Fix looks good. Function added