Popcorn contest - doublesharp's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L55 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L43 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L55

Vulnerability details

Impact

The AdapterBase.sol should disable initializers in the constructor so that the implementation cannot be initialized for inheriting adapters.

The contracts can be taken over by calling initialize() on the implementation with a strategy contract address that implements selfdestruct() in in the harvest() function.

Proof of Concept

Deploy strategy contract with a harvest function that self destructs

function harvest() {
  selfdestruct(payable(msg.sender));
}

Call Adapter.initialize() to become the implementation owner and pass in the self destructing strategy contract address

Call path:

• AdapterBase.deposit(uint256,address)
• AdapterBase.mint(uint256,address)
• AdapterBase.withdraw(uint256,address,address)
• AdapterBase.redeem(uint256,address,address)
• AdapterBase.harvest()
  • delegate calls the selfdestruct() function destroying the contract implementation

Tools Used

Slither, Forge

Recommended Mitigation Steps

Add constructor and disable initialisers in AdapterBase.sol

https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-_disableInitializers--

constructor() {
  _disableInitializers();
}

The owner arguments for Vault.initialize(), withdraw(), redeem() and permit() shadows the owner() function on OwnedUpgradeable

The owner argument should follow the same style as the other arguments to prevent shadowing the parent function. Using a different variable name/argument will prevent shadowing.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L57

Examples:

    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,
        address feeRecipient_,
        address owner_ // change here
    ) external initializer {
        // ...
        __Owned_init(owner_); // and here

    function withdraw(
        uint256 assets,
        address receiver,
        address assetOwner
    )

Vault contract implementation does not disable initializers

The Vault.sol contract should implement _disableInitializers() in its constructure to prevent implementation contracts from being initialized. As this contract does not use selfdestruct() the risk is minimized but could still lead to user confusion if it is taken over by a third party.

https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-_disableInitializers--

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L26

Proposed:

constructor() {
  _disableInitializers();
}