Popcorn contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L28 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/yearn/YearnAdapter.sol#L17

Vulnerability details

Impact

YearnAdapter previewRedeem / previewWithdraw does not consider the fee on yearn vault side.

Proof of Concept

According to the Yearn vault smart contract,

https://github.com/yearn/yearn-devdocs/blob/master/docs/developers/v2/SPECIFICATION.md#fees

the yearn vault charges fees:

The Treasury (which benefits Governance) collects a "management fee" based on the total assets the Vault has over a period of time, assessed each time the Strategy interacts with the Vault, and is provided as newly minted shares to the Treasury.

The Treasury (which benefits Governance) collects a "performance fee" based on the amount of returns a Strategy produces during Normal Operation, assessed each time the Strategy interacts with the Vault, and is provided as newly minted shares to the Treasury.

ach Strategist collects a "performance fee" based on the amount of positive returns their Strategy produces during Normal Operation, assessed each time the Strategy interacts with the Vault, and is provided as newly minted shares to the Strategist.

Performane fee, management fee are charged on Yearn vault side, but if we look into the implementation of YearnAdapter.sol,

contract YearnAdapter is AdapterBase {

which inherits from the regular ERC4646 inmplementation:

abstract contract AdapterBase is
    ERC4626Upgradeable,

the previewMint, previewRedeem, previewDeposit, previewWithdraw, does not consider the performance fee and maange fee charged.

This result that the preview related function over-estimate the available share can be minted nad available asset that can be withdraw / redeemed.

The result is quite miss leading and user can unexpected loss fund on the fee with interact with YearnAdapter.sol

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol estimate the management fee and performance fee in the preview related functoin in YearnAdapter.sol

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/DeploymentController.sol#L131 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/DeploymentController.sol#L121

Vulnerability details

Impact

Ownership change is incomplete

Proof of Concept

In the current implementation, the owner has a very crucial role. Owner can change important governance parameter such harvestCooldown, change implementation of clone, and add template. Owern can also ugprade the contract.

In the current implementation of the DeploymentController.sol, the implementation of two-step ownership change is implemented below:

  /**
   * @notice Nominates a new owner for dependency contracts. Caller must be owner. (`VaultController` via `AdminProxy`)
   * @param _owner The new `DeploymentController` implementation
   */
  function nominateNewDependencyOwner(address _owner) external onlyOwner {
    IOwned(address(cloneFactory)).nominateNewOwner(_owner);
    IOwned(address(cloneRegistry)).nominateNewOwner(_owner);
    IOwned(address(templateRegistry)).nominateNewOwner(_owner);
  }

  /**
   * @notice Accept ownership of dependency contracts.
   * @dev Must be called after construction.
   */
  function acceptDependencyOwnership() external {
    IOwned(address(cloneFactory)).acceptOwnership();
    IOwned(address(cloneRegistry)).acceptOwnership();
    IOwned(address(templateRegistry)).acceptOwnership();
  }

The ownerhsip of cloneFactory and cloneRegistry and templateRegistry is changed.

However, I think the ownership change is not completed if we hold the assumption that there should be only one multisig wallet or one EOA account that control the smart contract.

While the ownerhsip of cloneFactory and cloneRegistry and templateRegistry is updated, the ownership of PermissionedRegistry is not updated.

contract PermissionRegistry is Owned {

The ownerhsip of VaultController and VaultRegistry.sol is not updated as well.

contract VaultController is Owned {

and

contract VaultRegistry is Owned {

The impact can be minimal or severe. In the good case, the two ownership between vault registery and vault controller and permission registry make an agreement and update the ownership.

Let us say in the worst case, the old owner that control the CloneFactory.sol, CLoneRegister.sol and DeploymentController.sol and PermissionController.sol and TemplateRegistery.sol and Vault.sol and VaultController.sol and VaultRegistery.sol is hacked.

The hacker treis to perform a malicious upgrade or inject malicious template that can drain user's fund, a security memeber in protocol team detect hacker's transaction and use old owner's accoutn to front run the hacker's transaction by calling the nominateNewDependencyOwner and acceptDependencyOwnership.

Hacker's transaction is frontuned and the protocol reclaim the control of the cloneFactory.sol and cloneRegistry.sol and templateRegistry.sol.

However, the rest of the ownership is not updated, hacker sees that he still gain control of the VaultController.sol and VaultRegistery.sol and PermissionRegistry.sol.

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure The ownerhsip of VaultController and VaultRegistry.sol and PermissionedRegistry are updated as well if acceptDependencyOwnership is called on DeploymentController.sol

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L438

Vulnerability details

Impact

lastHarvest timestamp is not updated when harvest is called

Proof of Concept

In the current implementation of the AdapterBase.sol, the when harvest is called.

/**
 * @notice Execute Strategy and take fees.
 * @dev Delegatecall to strategy's harvest() function. All necessary data is passed via `strategyConfig`.
 * @dev Delegatecall is used to in case any logic requires the adapters address as a msg.sender. (e.g. Synthetix staking)
 */
function harvest() public takeFees {
	if (
		address(strategy) != address(0) &&
		((lastHarvest + harvestCooldown) < block.timestamp)
	) {
		// solhint-disable
		address(strategy).delegatecall(
			abi.encodeWithSignature("harvest()")
		);
	}

	emit Harvested();
}

lastHarvest is only set in the init function:

if (_strategy != address(0)) _verifyAndSetupStrategy(_requiredSigs);

highWaterMark = 1e18;
lastHarvest = block.timestamp;

Without updating the lastHarvest timestamp, after the first harvestCooldown is passed,

the check is lastHarvest + harvestCooldown) < block.timestamp is not useful anymore and it is bypassed every time when the harvest is called,

Then One of the grieving attack listed in the spec can happen.

• Set harvestCooldown too low and waste tokens and gas on harvests

Tools Used

Manual

Recommended Mitigation Steps

Update lastHarvest timestamp when harvest is called.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L578 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L525

Vulnerability details

Impact

legacy proposedAdapter and proposedFees state and proposedTimeStamp should be reset after timelock change applies

Proof of Concept

In the current implementation,

The timelock applies when new fee setting is proposed and when new adapter is proposed.

function proposeFees(VaultFees calldata newFees) external onlyOwner {
	if (
		newFees.deposit >= 1e18 ||
		newFees.withdrawal >= 1e18 ||
		newFees.management >= 1e18 ||
		newFees.performance >= 1e18
	) revert InvalidVaultFees();

	proposedFees = newFees;
	proposedFeeTime = block.timestamp;

	emit NewFeesProposed(newFees, block.timestamp);
}

/// @notice Change fees to the previously proposed fees after the quit period has passed.
function changeFees() external {
	if (block.timestamp < proposedFeeTime + quitPeriod)
		revert NotPassedQuitPeriod(quitPeriod);

	emit ChangedFees(fees, proposedFees);
	fees = proposedFees;
}

After the timelock period, anyone can call changeFees to apply the new fee setting.

However, the issue is that after the newFee is set, the proposedFees state is not cleaned up.

If a user wants to call changeFees, the transaction will suceed, the new fee setting will be updated again in the line of code:

fees = proposedFees;

This is very QA because no damage is done, however,

When the adapter address is changed, the legacy proposedAdapter address is not reset as well.

    /**
     * @notice Propose a new adapter for this vault. Caller must be Owner.
     * @param newAdapter A new ERC4626 that should be used as a yield adapter for this asset.
     */
    function proposeAdapter(IERC4626 newAdapter) external onlyOwner {
        if (newAdapter.asset() != address(asset))
            revert VaultAssetMismatchNewAdapterAsset();

        proposedAdapter = newAdapter;
        proposedAdapterTime = block.timestamp;

        emit NewAdapterProposed(newAdapter, block.timestamp);
    }

    /**
     * @notice Set a new Adapter for this Vault after the quit period has passed.
     * @dev This migration function will remove all assets from the old Vault and move them into the new vault
     * @dev Additionally it will zero old allowances and set new ones
     * @dev Last we update HWM and assetsCheckpoint for fees to make sure they adjust to the new adapter
     */
    function changeAdapter() external takeFees {
        if (block.timestamp < proposedAdapterTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        adapter.redeem(
            adapter.balanceOf(address(this)),
            address(this),
            address(this)
        );

        asset.approve(address(adapter), 0);

        emit ChangedAdapter(adapter, proposedAdapter);

        adapter = proposedAdapter;

        asset.approve(address(adapter), type(uint256).max);

        adapter.deposit(asset.balanceOf(address(this)), address(this));
    }

The damage however, can be quite severe. After the timelok, the user can repeated call

function changeAdapter() external takeFees {

every time this function is called,

first the adapter tries to redeem all shares, clean approval, then given approval and deposit the asset again and again.

Adapter can charge withdrawal fee if the adapter is BeefAdapter and adapters change performance fee and management fee if the adapter is YearnAdapter, in the future, it is likely that other integration of the adapter change fees as well.

An adversary can repeatedly call changeAdapter to drain the fund in the vault and the deposit and withdraw loss of the fee. and the fee adds up quickly.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol add onlyOwner modifier to make sure only owner can applies new fee changes and new adapters after the timelock and make sure reset the proposedAdapter to address(0) and reset the timelock timestamp to avoid such attack.

    /**
     * @notice Set a new Adapter for this Vault after the quit period has passed.
     * @dev This migration function will remove all assets from the old Vault and move them into the new vault
     * @dev Additionally it will zero old allowances and set new ones
     * @dev Last we update HWM and assetsCheckpoint for fees to make sure they adjust to the new adapter
     */
    function changeAdapter() external takeFees {
	
        if (block.timestamp < proposedAdapterTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);
			
			  // validation.
			  if(proposedAdapter; == address(0)) {
			     revert InvalidAdapter();
			  }

        adapter.redeem(
            adapter.balanceOf(address(this)),
            address(this),
            address(this)
        );

        asset.approve(address(adapter), 0);

        emit ChangedAdapter(adapter, proposedAdapter);

        adapter = proposedAdapter;

		    proposedAdapter = address(0) // reset propsoed adapter

        asset.approve(address(adapter), type(uint256).max);

        adapter.deposit(asset.balanceOf(address(this)), address(this));
    }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L253 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L134

Vulnerability details

Impact

User may get no shares when deposit in the vault.

Proof of Concept

In the current implementation, user can call Vault.sol#deposit to mint shares:

function deposit(uint256 assets, address receiver)
	public
	nonReentrant
	whenNotPaused
	syncFeeCheckpoint
	returns (uint256 shares)
{
	if (receiver == address(0)) revert InvalidReceiver();

	uint256 feeShares = convertToShares(
		assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
	);

	shares = convertToShares(assets) - feeShares;

	if (feeShares > 0) _mint(feeRecipient, feeShares);

	_mint(receiver, shares);

	asset.safeTransferFrom(msg.sender, address(this), assets);

	adapter.deposit(assets, address(this));

	emit Deposit(msg.sender, receiver, assets, shares);
}

However, the amount of shares depends on the total asset deposited in the vault.

function convertToShares(uint256 assets) public view returns (uint256) {
	uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

	return
		supply == 0
			? assets
			: assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
}

It is possible that the amount of share user can get be truncated to 0 if the totalAssets is large

shares = convertToShares(assets) - feeShares;

but because of the code does not revert when shares is 0, transaction can still go through:

_mint(receiver, shares);

asset.safeTransferFrom(msg.sender, address(this), assets);

adapter.deposit(assets, address(this));

User pays amount of asset, but get no shares.

Same missing 0 check applies to the redeem function. the function convertToAssets can return 0.

assets = convertToAssets(shares - feeShares);

If the shares is 0, transaction can still go through user can get no asset when burns shares to withdraw the asset.

uint256 feeShares = shares.mulDiv(
	uint256(fees.withdrawal),
	1e18,
	Math.Rounding.Down
);

assets = convertToAssets(shares - feeShares);

_burn(owner, shares);

if (feeShares > 0) _mint(feeRecipient, feeShares);

adapter.withdraw(assets, receiver, address(this));

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol check that the previewed share or withdraw amount is not 0 just like ERC4626 implementation in Somate does.

https://github.com/transmissions11/solmate/blob/1b3adf677e7e383cc684b5d5bd441da86bf4bf1c/src/mixins/ERC4626.sol#L48

    function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

and in Redeem function:

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual returns (uint256 assets) {
        if (msg.sender != owner) {
            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
        }

        // Check for rounding error since we round down in previewRedeem.
        require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");

note the check:

https://github.com/transmissions11/solmate/blob/1b3adf677e7e383cc684b5d5bd441da86bf4bf1c/src/mixins/ERC4626.sol#L107

require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");