Dopex - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

Vulnerability details

Impact

The PerpetualAtlanticVault.settle function does call PerpetualAtlanticVaultLP.subtractLoss function to account for the funds sent to RdpxV2Core. But due to the fact that the subtractLoss function requires strict equality between the contract collateral balance collateral.balanceOf(address(this)) and the actual collateral amount, a malicious user could transfer collateral tokens to the PerpetualAtlanticVaultLP contract to mess up the token balance (make it bigger than the actual contract accounting) and to make sure that subtractLoss will always revert causing a DOS of settle function.

Proof of Concept

The issue occurs in the settle function below :

function settle(
    uint256[] memory optionIds
)
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
{
    ...

    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
        addresses.perpetualAtlanticVaultLP,
        addresses.rdpxV2Core,
        ethAmount
    );
    // Transfer rdpx from rdpx rdpxV2Core to perpetual vault
    IERC20WithBurn(addresses.rdpx).safeTransferFrom(
        addresses.rdpxV2Core,
        addresses.perpetualAtlanticVaultLP,
        rdpxAmount
    );

    // @audit will call subtractLoss
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
        .subtractLoss(ethAmount);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
        .unlockLiquidity(ethAmount);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(
        rdpxAmount
    );

    emit Settle(ethAmount, rdpxAmount, optionIds);
}

As you can see after transferring ethAmount from PerpetualAtlanticVaultLP to RdpxV2Core the function will call subtractLoss to update the value of _totalCollateral in PerpetualAtlanticVaultLP.

The subtractLoss function is as follow :

function subtractLoss(uint256 loss) public onlyPerpVault {
    // @audit collateral.balanceOf(address(this)) can be changed by anyone
    require(
        collateral.balanceOf(address(this)) == _totalCollateral - loss,
        "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
}

The function uses a check statement that requires a strict equality between the contract collateral balance collateral.balanceOf(address(this)) and the actual collateral amount subtracted _totalCollateral - loss.

The problem here is that any address can change the value of collateral.balanceOf(address(this)) by sending a certain amount of WETH token to the PerpetualAtlanticVaultLP contract (simple ERC20 transfer which can be done by any user, even a very small amount) and thus any malicious user can use this trick (keep transferring small collateral funds to the contract) to make sure that the subtractLoss function always reverts resulting in the DOS of the settle function which will have a big negative impact on the protocol as the function is called by the admin to settle certain options which will not be possible.

Tools Used

Manual review

Recommended Mitigation Steps

To solve this issue i recommend to use the operator >= in the subtractLoss function to avoid the DOS even if a malicious user changes collateral.balanceOf(address(this)) :

function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
        collateral.balanceOf(address(this)) >= _totalCollateral - loss,
        "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L324-L336

Vulnerability details

Impact

In the UniV3LiquidityAMO contract there is a recoverERC721 function that allows the admin to recover the uni v3 NonfungiblePosition ERC721 tokens but the function will transfer the token to the RdpxV2Core contract instead of the admin as intended, because the RdpxV2Core contract has no way to interact with the ERC721 token and has no function to transfer it back to the admin this will lead the NonfungiblePosition ERC721 to be stuck forever in the RdpxV2Core contract and can never be transferred again.

Proof of Concept

The issue occurs in the recoverERC721 function below :

function recoverERC721(
    address tokenAddress,
    uint256 token_id
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Only the owner address can ever receive the recovery withdrawal
    // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported

    // @audit ERC721 sent to rdpxV2Core not to admin => will be stuck in rdpxV2Core for ever
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
        address(this),
        rdpxV2Core,
        token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
}

As we can see the recoverERC721 function does transfer the ERC721 token to the rdpxV2Core address (RdpxV2Core contract) even if the function comments states that "only the owner address can ever receive the recovery withdrawal".

By looking at the RdpxV2Core contract code we can easily notice that there is no way for the contract to interact or transfer ERC721 tokens (no call to ERC721.transferFrom/ERC721.safeTransferFrom), so any NonfungiblePosition ERC721 recovered will be stuck there and the admin will never get it back.

Tools Used

Manual review

Recommended Mitigation Steps

Transfer the recovered NonfungiblePosition ERC721 to the admin (which is msg.sender) :

function recoverERC721(
    address tokenAddress,
    uint256 token_id
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Only the owner address can ever receive the recovery withdrawal
    // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported
    // @audit Send token to admin == msg.sender
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
        address(this),
        msg.sender,
        token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
}

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990

Vulnerability details

Impact

The withdraw function within the RdpxV2Core contract permits delegates to retrieve unutilized WETH that they have previously supplied. However, although this function correctly transfers the designated amount to the user, it fails to decrement the totalWethDelegated balance. This discrepancy could lead to issues with the contract's functionalities, as it would appear to have more delegated funds than it actually possesses resulting in massive accounting error.

Proof of Concept

The issue occurs in the withdraw function below :

function withdraw(
    uint256 delegateId
) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

    // @audit Did not decrease totalWethDelegated amount

    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
}

As you can see the function calculate the amount that can be withdrawn amountWithdrawn by subtracting the active collateral delegate.activeCollateral from the user total delegated amount delegate.amount, the function then updates the value of delegate.amount and transfer the WETH amount to the user.

But the function did not decrease the totalWethDelegated balance which is supposed to account for all the WETH delegated to RdpxV2Core, and even this balance is increased when a user add delegated amount it is not decreased here when he is withdrawing, this will result is bad WETH balance accounting as the contract would appear to have more delegated funds than it actually possesses which will affect the protocol whenever totalWethDelegated is used.

For example if we take a look as the sync() function it has the following lines of code :

if (weth == reserveAsset[i].tokenAddress) {
    balance = balance - totalWethDelegated;
}
reserveAsset[i].tokenBalance = balance;

Because of the issue that as explained above, totalWethDelegated will be wrong and thus the contract WETH asset balance will be update incorrectly and will never reflect the real contract balance.

Tools Used

Manual review

Recommended Mitigation Steps

Update the totalWethDelegated balance in the withdraw function :

function withdraw(
    uint256 delegateId
) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

    // @audit subtract withdrawn amount from total weth delegated
    totalWethDelegated -= amountWithdrawn;

    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
}

Assessed type

Error

Judge has assessed an item in Issue #1553 as 2 risk. The relevant finding follows:

3- Funds not transferred to correct recipient in RdpxDecayingBonds.emergencyWithdraw : Risk : Low The function RdpxDecayingBonds.emergencyWithdraw is used by the admin to pull funds from the contract in case of emergency situation, the function allows the admin to specify a recipient to address as input to the function, this address should receive all the extracted funds (as explained in function Natspec comments). But due to an error in function code only the native token is transferred to to address and the other tokens are transferred to admin address.

This issue has a Low impact because there is no loss of funds and the tokens are transferred to admin which can later give them to the to address but it should be corrected.

Proof of Concept File: decaying-bonds/RdpxDecayingBonds.sol Line 89-107

function emergencyWithdraw( address[] calldata tokens, bool transferNative, address payable to, // @audit should be the recipient uint256 amount, uint256 gas ) external onlyRole(DEFAULT_ADMIN_ROLE) { _whenPaused(); if (transferNative) { (bool success, ) = to.call{value: amount, gas: gas}(""); require(success, "RdpxReserve: transfer failed"); } IERC20WithBurn token;

for (uint256 i = 0; i < tokens.length; i++) {
    token = IERC20WithBurn(tokens[i]);
    // @audit funds not transferred to `to` address 
    token.safeTransfer(msg.sender, token.balanceOf(address(this)));
}

} Mitigation Transfer all the funds to the correct recipient address to.

// @audit did not call IRdpxV2Core(rdpxV2Core).sync()

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

  // Send to custodian address
  univ3_positions.collect(collect_params);
}

QA Report

Summary

Findings

1- Potential underflow in RdpxV2Core.calculateBondCost will cause DOS of bond operations :

Risk : Low

The function RdpxV2Core.calculateBondCost is used to calculate the amount of weth and rdpx tokens required to create a bond, in the case where _rdpxBondId == 0 the function has to calculate the value of bondDiscount and it validate that its value is less than 100e8.

The issue is that in the following lines of code there is this substraction operation :

rdpxRequired =
    ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) *
        _amount *
        DEFAULT_PRECISION) /
    (DEFAULT_PRECISION * rdpxPrice * 1e2);

where RDPX_RATIO_PERCENTAGE = 25 * DEFAULT_PRECISION = 25e8

As you can see if we have bondDiscount / 2 > RDPX_RATIO_PERCENTAGE the operation will revert due to an underflow, and because the check present in the function only ensures that bondDiscount < 100e8, it means that there are many valid values for bondDiscount that will make the function revert. For example if we take bondDiscount = 60e8 (which is valid as it's less than 100e8) when the subtraction op goes on we will get :

RDPX_RATIO_PERCENTAGE - (bondDiscount / 2) = 25e8 - (60e8 / 2) = 25e8 - 30e8 < 0

And thus the operation will underflow causing the call to revert, and we can easily find that any value for bondDiscount greater than 50e8 will make the function revert.

When the calculateBondCost function revert because of the mentioned issue it will cause all the bonding functions (bond, bondWithDelegate) to revert hence blocking all the bonding operations (DOS) when _rdpxBondId == 0.

I submit this issue as Low risk for the protocol because we can see that the admin can change the value of bondDiscountFactor at any time to avoid this problem, but it could be a valid medium and the devs should take a look at it as if they are assuming in their logic that all values of bondDiscount less than 100e8 are possible (and hence the check : _validate(bondDiscount < 100e8, 14)) it will cause the protocol problems.

Because we can see that bondDiscount depends on the RDPX treasury reserve which will continue to grow meaning that the admin must be monitoring at each time if the value of bondDiscount is getting bigger than 50e8 in order to change the bondDiscountFactor to avoid the DOS of bonding operation (revert of the call due to underflow), this is not really an ideal scenario.

Proof of Concept

File: core/RdpxV2Core.sol Line 1156-1199

function calculateBondCost(
    uint256 _amount,
    uint256 _rdpxBondId
) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
    uint256 rdpxPrice = getRdpxPrice();
    if (_rdpxBondId == 0) {
        uint256 bondDiscount = (bondDiscountFactor *
            Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
            1e2) / (Math.sqrt(1e18)); // 1e8 precision

        // @audit allow values of bondDiscount < 100e8 
        _validate(bondDiscount < 100e8, 14);

        // @audit can underflow if bondDiscount > 50e8
        rdpxRequired =
            ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) *
                _amount *
                DEFAULT_PRECISION) /
            (DEFAULT_PRECISION * rdpxPrice * 1e2);

        wethRequired =
            ((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /
            (DEFAULT_PRECISION * 1e2);
    } else {
        ...
}

Mitigation

There are many options the solve this issue but the simpler is to modify the check on the amount bondDiscount and make it always less tha 50e8 : _validate(bondDiscount < 50e8, 14)

2- Missing sync() after transferring funds to RdpxV2Core :

Risk : Low

In the RdpxV2Core contract there a sync() function which allows the update of the state variables tracking the tokens balances (ETH, DPX,...) by setting them equal to the actual contract token balance (with the use of token.balanceOf(address(this))).

The issue is that there are some instances where this function must be called after transferring funds to RdpxV2Core but it is not which could result in a temporarily incorrect balance accounting.

Proof of Concept

There 2 instances of this issue :

File: amo/UniV2LiquidityAmo.sol Line 160-178

function _sendTokensToRdpxV2Core() internal {
    uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
      address(this)
    );
    uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
      address(this)
    );
    // transfer token A and B from this contract to the rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      tokenABalance
    );
    IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      tokenBBalance
    );

    // @audit did not call IRdpxV2Core(rdpxV2Core).sync()

    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
}

The _sendTokensToRdpxV2Core() function is used many times in the UniV2LiquidityAmo to transfer tokens to RdpxV2Core but it doesn't call the sync() function after doing so, we can see that a similar function is present in the other AMO contract UniV3LiquidityAmo._sendTokensToRdpxV2Core and this one does call the sync() function after the transfers.

File: amo/UniV3LiquidityAmo.sol Line 119-133

function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
    for (uint i = 0; i < positions_array.length; i++) {
      Position memory current_position = positions_array[i];
      INonfungiblePositionManager.CollectParams
        memory collect_params = INonfungiblePositionManager.CollectParams(
          current_position.token_id,
          rdpxV2Core,
          type(uint128).max,
          type(uint128).max
        );

      // Send to custodian address
      univ3_positions.collect(collect_params);
    }
}

In the second instance the call to univ3_positions.collect will transfer tokens to the RdpxV2Core contract but after the loop there no call to the sync() function to update the tokens accounting in RdpxV2Core.

Mitigation

Add the IRdpxV2Core(rdpxV2Core).sync() call to the instances aforementioned to make sure that the accounting is always correct after a transfer from the AMOs contracts.

3- Funds not transferred to correct recipient in RdpxDecayingBonds.emergencyWithdraw :

Risk : Low

The function RdpxDecayingBonds.emergencyWithdraw is used by the admin to pull funds from the contract in case of emergency situation, the function allows the admin to specify a recipient to address as input to the function, this address should receive all the extracted funds (as explained in function Natspec comments). But due to an error in function code only the native token is transferred to to address and the other tokens are transferred to admin address.

This issue has a Low impact because there is no loss of funds and the tokens are transferred to admin which can later give them to the to address but it should be corrected.

Proof of Concept

File: decaying-bonds/RdpxDecayingBonds.sol Line 89-107

function emergencyWithdraw(
    address[] calldata tokens,
    bool transferNative,
    address payable to, // @audit should be the recipient
    uint256 amount,
    uint256 gas
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _whenPaused();
    if (transferNative) {
        (bool success, ) = to.call{value: amount, gas: gas}("");
        require(success, "RdpxReserve: transfer failed");
    }
    IERC20WithBurn token;

    for (uint256 i = 0; i < tokens.length; i++) {
        token = IERC20WithBurn(tokens[i]);
        // @audit funds not transferred to `to` address 
        token.safeTransfer(msg.sender, token.balanceOf(address(this)));
    }
}

Mitigation

Transfer all the funds to the correct recipient address to.

4- UniV2LiquidityAMO.approveContractToSpend can't approve USDT transfer :

Risk : Low

In the UniV2LiquidityAMO contract, the function approveContractToSpend is used to approve other contracts to transfer funds outside UniV2LiquidityAMO, for doing so the function uses the approve function and there is a check ensuring that the approved amount is always greater than zero, this won't allow the approval of USDT token transfers as in that case it must first reset the approval amount to zero before setting a new one.

Proof of Concept

File: amo/UniV2LiquidityAmo.sol Line 126-135

function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(_token != address(0), "reLPContract: token cannot be 0");
    require(_spender != address(0), "reLPContract: spender cannot be 0");
    // @audit reverts if amount == 0
    require(_amount > 0, "reLPContract: amount must be greater than 0");
    // @audit rcan not be used for USDT
    IERC20WithBurn(_token).approve(_spender, _amount);
}

Mitigation

Use the safeApprove function to allow USDT tokens approval, the contract should implement a function similair to UniV3LiquidityAMO.approveTarget.

5- PerpetualAtlanticVaultLP can never pull funds from PerpetualAtlanticVault :

Risk : Low

The PerpetualAtlanticVaultLP contract is approved to pull collateral tokens from PerpetualAtlanticVault and there is even a function setLpAllowance that can be called by the admin to increase/decrease the allowance approved for PerpetualAtlanticVaultLP, but by looking at the PerpetualAtlanticVaultLP contract code we can notice that there no call to pull funds from PerpetualAtlanticVault which means that PerpetualAtlanticVaultLP can never take the funds that are approved from PerpetualAtlanticVault.

This does not make sense to approve a contract to spend funds but in its code there no way to do so, this could be a critical issue and should be looked at, if the PerpetualAtlanticVaultLP does need to pull funds from PerpetualAtlanticVault then the whole contract code should be reviewed to enable it (in the other case there no need to have setLpAllowance function or to approve PerpetualAtlanticVaultLP).

Instance

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L207-L210

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L243-L250

6- MANAGER_ROLE not used in PerpetualAtlanticVault :

Risk : Low

The MANAGER_ROLE is set in the constructor of the PerpetualAtlanticVault contract but this role is never used inside the contract for any access control task, If this role is not intended to be used in the contract then it should be removed, but if it was supposed to have a certain power for accessing some functions inside the contract then the code should be reviewed to add include this role.

Instance

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L127

7- underlyingSymbol not set in PerpetualAtlanticVaultLP :

Risk : Non critical

The underlyingSymbol variable is not set in the constructor of PerpetualAtlanticVaultLP, there is a string variable that is computed from the concatenation of the collateral token symbol and the suffix "-LP" but this is assigned to the inherited ERC20 symbol variable and underlyingSymbol will remain empty as there is no way to set it after deployment.

Instance

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L52

8- Unused state variable collateralPrecision in the PerpetualAtlanticVault contract :

Risk : Non critical

The value of collateralPrecision is set in the constructor of the PerpetualAtlanticVault contract but its value is never used afterwards in any parts of the contract, if this variable is not intended to be used in the contract then it should be removed, but if it was supposed to used in some calculation in a certain function (as far as i saw it has no use as only 18 decimals tokens are intended to be used) then the code should be reviewed to add this features.

Instance

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L123