[Lines of code](https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/zaps/PoolMigrationZap.sol#L61) # Vulnerability details ## Impact A malicious admin can steal user funds or lock their balances forever Even if the user is benevolent the fact that there is a rug vector available may [negatively impact the protocol's reputation](). ## Proof of Concept Unlike the original Convex code that goes to great lengths to prevent users having the ability to transfer funds/mint things, this project introduces multiple roles and new abilities that require users to place more trust in governance: 1. Admins can initiate migrations and set the newPool_ to be a contract that forwards funds to accounts they control File: protocol/contracts/zaps/PoolMigrationZap.sol #1 61 ILiquidityPool newPool_ = _underlyingNewPools[underlying_]; 62 uint256 ethValue_ = underlying_ == address(0) ? underlyingAmount_ : 0; 63 newPool_.depositFor{value: ethValue_}(msg.sender, underlyingAmount_); 2. Admins can add infinite newRewardTokens: File: protocol/contracts/BkdLocker.sol #2 70 function migrate(address newRewardToken) external override onlyGovernance { 71 _replacedRewardTokens.remove(newRewardToken); 72 _replacedRewardTokens.set(rewardToken, block.timestamp); 73 lastMigrationEvent = block.timestamp; 74 rewardToken = newRewardToken; 75 } so that _userCheckpoint()s, which are required to withdraw funds, revert because they run out of gas iterating over the tokens: File: protocol/contracts/BkdLocker.sol #3 292 function _userCheckpoint( 293 address user, 294 uint256 amountAdded, 295 uint256 newTotal 296 ) internal { 297 RewardTokenData storage curRewardTokenData = rewardTokenData[rewardToken]; 298 299 // Compute the share earned by the user since they last updated 300 uint256 userBalance = balances[user]; 301 if (userBalance > 0) { 302 curRewardTokenData.userShares[user] += (curRewardTokenData.feeIntegral - 303 curRewardTokenData.userFeeIntegrals[user]).scaledMul( 304 userBalance.scaledMul(boostFactors[user]) 305 ); 306 307 // Update values for previous rewardTokens 308 if (lastUpdated[user] < lastMigrationEvent) { 309 uint256 length = _replacedRewardTokens.length(); 310 for (uint256 i; i < length; i = i.uncheckedInc()) { 3. Admins can set a malicious feeBurner, via the addressProvider, which just takes the fees for itself File: protocol/contracts/RewardHandler.sol #4 35 function burnFees() external override { 36 IBkdLocker bkdLocker = IBkdLocker(addressProvider.getBKDLocker()); 37 IFeeBurner feeBurner = addressProvider.getFeeBurner(); 38 address targetLpToken = bkdLocker.rewardToken(); 39 address[] memory pools = addressProvider.allPools(); 40 uint256 ethBalance = address(this).balance; 41 address[] memory tokens = new address[](pools.length); 42 for (uint256 i; i < pools.length; i = i.uncheckedInc()) { 43 ILiquidityPool pool = ILiquidityPool(pools[i]); 44 address underlying = pool.getUnderlying(); 45 if (underlying != address(0)) { 46 _approve(underlying, address(feeBurner)); 47 } 48 tokens[i] = underlying; 49 } 50 feeBurner.burnToTarget{value: ethBalance}(tokens, targetLpToken); 4. Admins can set an oracle that provides the wrong answers: File: protocol/contracts/swappers/SwapperRouter.sol #5 452 try _addressProvider.getOracleProvider().getPriceETH(token_) returns (uint256 price_) { ## Tools Used Code inspection ## Recommended Mitigation Steps The trust-minimizing approach that Convex took was to not allow admins to change addresses. In order for things to change, an admin is allowed to completely shut everything down, and during the shut down state, users are still able to withdraw their funds. Later, the admins spin up a whole new set of contracts, and let users migrate things themselves. Something similar can be done here by having the DAO accept proposals to spawn specific contracts, and hook up specific addresses in certain ways in the new deployment --- The text was updated successfully, but these errors were encountered: All reactions