Zellic - Audit Reports

Notable observations

In PerpPositionHandler: There are no events for opening and closing positions. You might want to consider adding them.

function _openPosition(bytes calldata data) internal override {
    OpenPositionParams memory openPositionParams = abi.decode(
        data,
        (OpenPositionParams)
    );
    bytes memory L2calldata = abi.encodeWithSelector(
        IPositionHandler.openPosition.selector,
        openPositionParams._isShort,
        openPositionParams._amount,
        openPositionParams._slippage
    );

    sendMessageToL2(
        positionHandlerL2Address,
        L2calldata,
        openPositionParams._gasLimit
    );
}

function _closePosition(bytes calldata data) internal override {
    ClosePositionParams memory closePositionParams = abi.decode(
        data,
        (ClosePositionParams)
    );
    bytes memory L2calldata = abi.encodeWithSelector(
        IPositionHandler.closePosition.selector,
        closePositionParams._slippage
    );
    sendMessageToL2(
        positionHandlerL2Address,
        L2calldata,
        closePositionParams._gasLimit
    );
}

In library/AddArrayLib.sol, the removeAddress() function is documented as returning boolean, but it returns nothing.

/**
 * @notice remove an address from the array
 * @dev finds the element, swaps it with the last element, and then deletes it;
 *      returns a boolean whether the element was found and deleted
 * @param self Storage array containing address type variables
 * @param element the element to remove from the array
 */
function removeAddress(Addresses storage self, address element) internal {
    for (uint256 i = 0; i < self.size(); i++) {
        if (self._items[i] == element) {
            self._items[i] = self._items[self.size() - 1];
            self._items.pop();
        }
    }
}

Here is an interesting observation about how fees are charged: Let's say 1000 USDC are deposited into the protocol. The executors manage to lose 500 USDC. The next deposit or withdrawal happens, no fee is charged. Then the executors manage to gain 100 USDC. The next deposit or withdrawal happens, a fee is charged on that 100-USDC "gain". The stakers have effectively lost 400 USDC but still have to pay a fee.

Lax access control in token withdrawals from Harvester: If someone accidentally deposits token into it, anyone can take them away. This is mitigated by the fact that the default user of Harvester, ConvexTradeExecutor, never leaves tokens in Harvester.

/// @notice Harvest the entire swap tokens list, i.e convert them into wantToken
/// @dev Pulls all swap token balances from the msg.sender, swaps them into wantToken, and sends back the wantToken balance
function harvest() external override {
    uint256 crvBalance = crv.balanceOf(address(this));
    uint256 cvxBalance = cvx.balanceOf(address(this));
    uint256 _3crvBalance = _3crv.balanceOf(address(this));
...
    // send token usdc back to vault
    IERC20(vault.wantToken()).safeTransfer(
        msg.sender,
        IERC20(vault.wantToken()).balanceOf(address(this))
    );
}

In ConvexPositionHandler.sol, to protect against any possible Curve manipulations, the usage of virtual price has to be taken into account when calcuating the positionInWantToken, and for that we need to set the parameter to true. Instead of relying on on-chain AMM's pricing when calculating the value of staked and LP tokens in this case, using the virtual_price offered by Curve does not allow for any possible manipulations that an attacker could ever cause.

...
(
    uint256 stakedLpBalance,
    uint256 lpTokenBalance,
    uint256 usdcBalance
) = _getTotalBalancesInWantToken(true);

Although the usage of the virtual price was set in the _withdraw() function, we also have to take the minimum of the calculated and actual staked balance, such that no overflow can occur:

function _withdraw(bytes calldata _data) internal override {
    ...
    uint256 lpTokensToUnstake = _USDCValueInLpToken(
        amountToUnstake
    ) > baseRewardPool.balanceOf(address(this))
        ? baseRewardPool.balanceOf(address(this))
        : _USDCValueInLpToken(amountToUnstake);
    ...

The protection also applies in the case of calculating the amount of LP tokens to convert into USDC:

function _withdraw(bytes calldata _data) internal override {
    ...
    uint256 lpTokensToConvert = _USDCValueInLpToken(
        usdcValueOfLpTokensToConvert
    ) > lpToken.balanceOf(address(this))
        ? lpToken.balanceOf(address(this))
        : _USDCValueInLpToken(usdcValueOfLpTokensToConvert);
    ...

This has been found and properly mitigated by the Brahma team.