Admin of the upgradeable proxy contracts can rug users

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Managed.sol#L18 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Managed.sol#L157 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/DripsHub.sol#L53 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/AddressDriver.sol#L19 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/NFTDriver.sol#L19 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/ImmutableSplitsDriver.sol#L11

Vulnerability details

Impact

The proxy admin can steal all the tokens approved to the proxy contract from users' wallets. Contracts DripsHub, AddressDriver, NFTDriver have this vulnerability. The proxy admin of the drivers can steal its users' tokens in the DripsHub. Contracts AddressDriver and NFTDriver have this vulnerability. The proxy admin of DripsHub can steal all users' and drivers' tokens in it.

Proof of Concept

All of contracts DripsHub, AddressDriver, NFTDriver, ImmutableSplitsDriver inherit from the Managed contract, and will be deployed as a implementation of an upgradeable ManagedProxy contract. The admin of the ManagedProxy contract can upgrade the implementation to any contract. A hacked admin or malicious admin can perform arbitrary malicious actions by upgrading the contract.

For DripsHub:

• Steal all tokens in the hub, which includes all uncollected assets of all users in the protocol:
  1. Upgrade the DripsHub to a custom malicious contract
  2. Let the upgraded contract call ERC20.transfer() to transfer the tokens from the hub to hacker's address one by one (or in batch)
• Steal tokens from users' wallets or drivers:
  1. Upgrade the DripsHub to a custom malicious contract
  2. Let the upgraded contract call ERC20.transferFrom() to transfer any token from any user or driver who allows the hub to spend the token

For AddressDriver, NFTDriver:

• Steal tokens from users' wallets:
  1. Upgrade the driver to a custom malicious contract
  2. Let the upgraded contract call ERC20.transferFrom() to transfer any token from any user's wallet who allows the driver to spend the token
• Steal users' tokens in DripsHub
  1. Upgrade the driver to a custom malicious contract
  2. Let the upgraded contract call DripsHub.setDrips() to drop all remain balance to the hacker's address
  3. Let the upgraded contract call DripsHub.setSplits() to split its users' splittable tokens to hacker's userId
  4. Let the upgraded contract call DripsHub.collect() to collect its users' tokens in DripsHub and then transfer the tokens to the hacker's address

For ImmutableSplitsDriver:

• Steal users' tokens in DripsHub
  1. Upgrade the driver to a custom malicious contract
  2. Let the upgraded contract call DripsHub.setSplits() to split its users' splittable tokens to hacker's userId

Tools Used

VS Code

Recommended Mitigation Steps

We should use a non-upgradeable contract to hold user's allowances.

For all of the official drivers(AddressDriver, NFTDriver, ImmutableSplitsDriver), consider removing the upgradability.

For DripsHub:

• Consider removing the upgradability
• Consider adding a time lock to the upgrade function

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope

[GalloDaSballo]

Upgradeability per-se is OOS