search

code-423n4 / 2021-07-pooltogether-findings

0 stars 0 forks source link

Single-step process for critical ownership transfer/renounce is risky #40

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The SwappableYieldSource allows owners and asset managers to set/swap/transfer yield sources/funds. As such, the contract ownership plays a critical role in the protocol.

Given that AssetManager is derived from Ownable, the ownership management of this contract defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes.

Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() or onlyOwnerOrAssetManager() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.

Proof of Concept

See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf

See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/081776bf5fae2122bfda8a86d5369496adfdf959/contracts/access/OwnableUpgradeable.sol#L59-L76

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/access/AssetManager.sol#L20-L61

Tools Used

Manual Analysis

Recommended Mitigation Steps

Override the inherited methods to null functions and use separate functions for a two-step address change: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.

Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.

PierrickGT commented 3 years ago

This isn't a security issue but an improper use of the initialize function. We do check for address zero so at least the risk of deploying the contract with address zero is excluded. Also, these contracts will be deployed by a multi sig owned by governance so the risk of a single human error is almost null.

0xean commented 3 years ago

Disagree with sponsor. A two step process would be a safer implementation. A multi-sig does not remove human error or the potential risk here. It may be an acceptable risk to the team, but still worth highlighting with the given severity.

PierrickGT commented 3 years ago

We have studied this solution and decided to not implement it since it would make it a pretty tedious process to deploy a swappable yield source, especially through the use of our builder which would mean that a user would have to manually claimOwnership after deploying a pool. Plus, this contract will be owned by governance so it will be very difficult to transfer it to another owner or renounce ownership.