Popcorn contest - 0xTraub's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L80

Vulnerability details

Impact

Vault creator can initialize a BeefyAdapter with a booster address that allows them to rug the entirety of the BeefyAdapter. The system checks that the template being deployed is endorsed, but does not check the validity of the parameters it is being initialized with. When deploying a BeefyAdapter the user has the option to initialize with a Booster Address for harvesting additional rewards. There is no validation to ensure that this is a legitimate Booster contract with Beefy, and it is given an unlimited allowance to transfer the Moo tokens held by the adapter. It only requires that certain functions exist so that a deposit can succeed but that's it. A user could deploy the existing endorsed-template and initialize with a booster contract controlled by them. This contract could appear legitimate by proxying all of the booster functionality to accrue value to the adapter. Then they would simply activate the functionality that transfers away all of the Moo tokens to an external address owned by them, at which the vault is now insolvent.

Proof of Concept

https://gist.github.com/jhweintraub/8685c451e0113af8651cb56b295a0ca8

Tools Used

Foundry

Recommended Mitigation Steps

Include input validation on various external contracts designated at initialization. Consider using a permission registry of allowed Booster contracts and ensure that the provided one is whitelisted/not_blacklisted.

Lines of code

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

Vulnerability details

Impact

The high water mark is used by Vault.sol to determine whether or not to take a performance fee. If the value of convertToAssets(1e18) > HWM and performanceFee > 0 then a fee is taken. However, highWaterMark is default set to 1e18 in the code. As a result a token that has less than 18 decimals of precision will almost always result in a convertToAssets() value <1e18, in which case no fee will be collected even as the vault/adapter accumulates value. This occurs despite the use of syncFeeCheckpoint(). This

Proof of Concept

https://gist.github.com/jhweintraub/f1c420711ff429bacd3e52fd1afa241d

Tools Used

Recommended Mitigation Steps

The value of highWaterMark should be set in the initialize() function as based on the underlying decimals of the want token.

highWaterMark = IERC20(asset).decimals();

1. Pool2SingleAssetCompounder.sol -> Adapter may be ruled incompatible for an adapter in which a single-hop trade router does not exist even when a 2-step route does. This limits the applicability of adapters that would fail to initialize because no such hop exists. Consider using an external contract for trade routers or allowing a user to submit their own

   tradePath[0] = rewardTokens[i];

   uint256[] memory amountsOut = IUniswapRouterV2(router).getAmountsOut(ERC20(asset).decimals() ** 10, tradePath); if (amountsOut[amountsOut.length] == 0) revert NoValidTradePath();

2. By default all tokens will be allowed until otherwise denied. This is bad opsec and should be limited at first and then more tokens allowed later.

function _verifyToken(address token) internal view { if ( ( permissionRegistry.endorsed(address(0)) ? !permissionRegistry.endorsed(token) : permissionRegistry.rejected(token)

3. MultiRewardEscrow.sol -> if fee amount is zero due to rounding the 1e18 in the calculation then an error NoFee should be thrown if (feePerc > 0) { uint256 fee = Math.mulDiv(amount, feePerc, 1e18);

   amount -= fee; token.safeTransfer(feeRecipient, fee); }

Similarly, Error NoFee(IERC20) is never used. If accidental then function setFees() should check that each fee is nonzero when executing a new fee, otherwise remove dead-code.

4. MultiRewardEscrow.sol -> block.timestamp can be manipulated by validators. Consider using block.number instead, with duration equaling number_of_slots * seconds_per_slot (12)

5. AdapterBase.sol -> withdraw() and redeem() not tagged nonReentrant. While vault does, the adapter makes calls to external contracts not owned by the protocol/user and so extra care should be applied