`ImmutableSplitsDriver` is not immutable

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/ImmutableSplitsDriver.sol#L11

Vulnerability details

The ImmutableSplitsDriver contract is considered to implement immutable splits. However, as the contract inherits from the Managed contract, which allows the contract admin to upgrade the contract, splits are not immutable.

Impact

The admin of the ImmutableSplitsDriver contract can upgrade the contract to add functionality that allows updating previously considered immutable splits.

Proof of Concept

ImmutableSplitsDriver.sol#L11

11: contract ImmutableSplitsDriver is Managed {

Managed.sol#L18

The Managed contract inherits from the UUPSUpgradeable contract, which allows the contract admin to upgrade the contract.

18: abstract contract Managed is UUPSUpgradeable {

scripts/deploy.sh#L141-L154

The ImmutableSplitsDriver contract is deployed in the deploy.sh script with the ManagedProxy contract, which allows the contract admin (i.e., SPLITS_DRIVER_ADMIN - governance address) to upgrade the contract.

141: if [ -z "$SPLITS_DRIVER" ]; then
142:     if [ -z "$SPLITS_DRIVER_LOGIC" ]; then
143:         NONCE=$(($(cast nonce $DEPLOYER) + 2))
144:         SPLITS_DRIVER=$(cast compute-address $DEPLOYER --nonce $NONCE | cut -d " " -f 3)
145:         SPLITS_DRIVER_ID=$(cast call "$DRIPS_HUB" 'nextDriverId()(uint32)')
146:         send "Registering ImmutableSplitsDriver in DripsHub" \
147:             "$DRIPS_HUB" 'registerDriver(address)(uint32)' "$SPLITS_DRIVER"
148:         create "ImmutableSplitsDriver logic" 'src/ImmutableSplitsDriver.sol:ImmutableSplitsDriver' \
149:             "$DRIPS_HUB" "$SPLITS_DRIVER_ID"
150:         SPLITS_DRIVER_LOGIC=$DEPLOYED_ADDR
151:     fi
152:     create "ImmutableSplitsDriver" 'src/Managed.sol:ManagedProxy' \
153:         "$SPLITS_DRIVER_LOGIC" "$SPLITS_DRIVER_ADMIN"
154:     SPLITS_DRIVER=$DEPLOYED_ADDR

Tools Used

Manual review

Recommended mitigation steps

Consider removing the upgrade functionality from the ImmutableSplitsDriver contract.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[xmxanuel]

We had an internal discussion about it. Governance could upgrade the contracts but even if the ImmutableSplitsDriver is not upgradeable we could upgrade the DripsHub contract and introduce special handling for the ImmutableSplitsDriver.

In general, it is a trade-off of Governance itself.

[CodeSandwich]

[disagree with severity: QA or dispute validity] What Manuel said, the protocol governance's good will is necessary for the entire thing to work. If the governance decides to freeze any of the Managed contracts, their ownership can always be given to the zero address.

[c4-sponsor]

CodeSandwich marked the issue as disagree with severity

[GalloDaSballo]

I believe the instance of Admin Privilege to be downgradeable because of public disclosure + a public V1

That said I think the finding has some validity, am thinking QA Low as a Gotcha

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c

[c4-judge]

GalloDaSballo marked the issue as grade-a