Dopex - kodyvim's results

Lines of code

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

Vulnerability details

Impact

PerpetualAtlanticVaultLP.subtractLoss uses strict equal to check if enough collateral was sent out following the call to PerpetualAtlanticVault.settle.

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

_totalCollateral tracks the total collateral available in the contract but does not account for values sent directly to the contract. when making settlement in PerpetualAtlanticVault.settle it checks that the amount transferred from perpetual vault to rdpxV2Core is accurate by calling subtractLoss at the end of the transaction.

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

    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );//@audit-info
    ...SNIP

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );//@audit-issue

    ...SNIP

    emit Settle(ethAmount, rdpxAmount, optionIds);
  }

if an attacker sends as little as 1 wei of collateral token to perpetualAtlanticVaultLp _totalCollateral would be out of sync with it's actual contract balance causing the call to always revert.

Proof of Concept

Adjust the following test /tests/perp-vault/Unit.t.sol#testSettle to:

function testSettle() public {
    weth.mint(address(1), 1 ether);
+   weth.mint(address(1337), 1 wei);
    deposit(1 ether, address(1));

    vault.purchase(1 ether, address(this));

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;
    vm.expectRevert(
      abi.encodeWithSelector(
        PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
        7
      )
    );
    vault.settle(ids);

    priceOracle.updateRdpxPrice(0.2 gwei); // initial price * 10
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;
    skip(86500); // expire
    vm.expectRevert(
      abi.encodeWithSelector(
        PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
        7
      )
    );
    vault.settle(ids);

+   vm.prank(address(1337));
+   weth.transfer(address(vaultLp), 1 wei);//@audit attacker sends 1 wei of collateral token to the contract

    priceOracle.updateRdpxPrice(0.010 gwei); // ITM
    uint256 wethBalanceBefore = weth.balanceOf(address(this));
    uint256 rdpxBalanceBefore = rdpx.balanceOf(address(this));
+   vm.expectRevert("Not enough collateral was sent out");//@audit All settlement would fail
    vault.settle(ids);
    uint256 wethBalanceAfter = weth.balanceOf(address(this));
    uint256 rdpxBalanceAfter = rdpx.balanceOf(address(this));

    assertEq(wethBalanceAfter - wethBalanceBefore, 0.15 ether); // asset settled; 1 rdpx worth of ether received by rdpxV2Core
    assertEq(rdpxBalanceBefore - rdpxBalanceAfter, 1 ether); // asset settled; 1 rdpx sent from rdpxV2Core to lp
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider removing the strict equality check in subtractLoss because the check is redundant since token transfer from perpetual vault to rdpxV2Core uses safe transfer which would actually revert if the transfer fails.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

ERC721 tokens sent to RdpxV2Core could be stuck

Proof of Concept

ERC721 tokens can be recovered to RdpxV2Core from Amo contracts. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L324

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
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
      address(this),
      rdpxV2Core,
      token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
  }

but there is no function available to transfer these tokens to the appropriate recipient in RdpxV2Core. These tokens could be stuck within the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a functionality to transfer ERC721 tokens from RdpxV2Core with proper access control.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

syncing would fail following every withdrawal. RdpxV2Core.sync syncs asset reserves with the contract balance, this is used to update reserveAsset.tokenBalance which tracks the balance of asset owned by the contract. sync may be called after balance changes to update the state of asset balance. The issue is that when a user addToDelegate in RdpxV2Core, totalWethDelegated is incremented by the amount supplied. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L941

function addToDelegate(
    uint256 _amount,
    uint256 _fee
  ) external returns (uint256) {
   ...SNIP

    Delegate memory delegatePosition = Delegate({
      amount: _amount,
      fee: _fee,
      owner: msg.sender,
      activeCollateral: 0
    });
    delegates.push(delegatePosition);

    // add amount to total weth delegated
    totalWethDelegated += _amount;//@audit-info

    emit LogAddToDelegate(_amount, _fee, delegates.length - 1);
    return (delegates.length - 1);
  }

but if the user withdraw, totalWethDelegated is not decremented. This causes sync to revert since it adjust totalWethDelegated from it's balance. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L995

function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

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

    emit LogSync();
  }

From the AMO contracts sync is called when adding or removing liquidity through _sendTokensToRdpxV2Core https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L361

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

    // sync token balances
    IRdpxV2Core(rdpxV2Core).sync();//@audit this call may fail

    emit LogAssetsTransfered(tokenABalance, tokenBBalance, tokenA, tokenB);
  }

This would prevent the addition/removal of liquidity from uniswapNonfungiblePositionManager since the call to sync could revert. this would also affect any other function calls within the protocol that relies on sync.

Sync fails after every withdraw since totalWethDelegated deviates from the actual total WETH delegated to the contract.

This could be triggered intentionally or unintentionally by users. A malicious user could takeout a flashloan and call addToDelegate and also withdraw within the same transaction to greatly inflate the value of totalWethDelegated.

Proof of Concept

add the following line to /tests/Unit.t.sol#testWithdraw

function testWithdraw() public {
    rdpxV2Core.addToDelegate(1 * 1e18, 10e8);

    // test withdraw with invalid delegate id
    vm.expectRevert(
      abi.encodeWithSelector(RdpxV2Core.RdpxV2CoreError.selector, 14)
    );
    rdpxV2Core.withdraw(1);

    // test withdraw without ownership
    vm.expectRevert(
      abi.encodeWithSelector(RdpxV2Core.RdpxV2CoreError.selector, 9)
    );
    vm.prank(address(1), address(1));
    rdpxV2Core.withdraw(0);

    // test withdraw successfully
    uint256 userBalance = weth.balanceOf(address(this));
    rdpxV2Core.withdraw(0);
    assertEq(weth.balanceOf(address(this)), userBalance + 1 * 1e18);
    (, uint256 amount, , uint256 activeCollateral) = rdpxV2Core.delegates(0);
    assertEq(amount, 0);
    assertEq(activeCollateral, 0);

+   //@audit test sync would fail after each withdraw
+   vm.expectRevert();//[FAIL. Reason: Arithmetic over/underflow]
+   rdpxV2Core.sync();

    // test withdraw with 0 amount
    vm.expectRevert(
      abi.encodeWithSelector(RdpxV2Core.RdpxV2CoreError.selector, 15)
    );
    rdpxV2Core.withdraw(0);

    // test partial amount
    rdpxV2Core.addToDelegate(2 * 1e18, 10e8);
    uint256[] memory _amounts = new uint256[](1);
    uint256[] memory _delegateIds = new uint256[](1);
    _delegateIds[0] = 1;
    _amounts[0] = 2 * 1e18;
    userBalance = weth.balanceOf(address(this));
    (, amount) = rdpxV2Core.calculateBondCost(2e18, 0);
    rdpxV2Core.bondWithDelegate(address(this), _amounts, _delegateIds, 0);
    rdpxV2Core.withdraw(1);
    assertEq(weth.balanceOf(address(this)), userBalance + (2e18 - amount));
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Adjust totalWethDelegated when delegated weth are withdrawn.

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;

+   // remove amount from total weth delegated
+   totalWethDelegated -= amountWithdrawn;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Context

counters.sol is deprecated

counters used in RdpxDecayingBonds and perpetualAtlanticVault would be removed from recent openzepplin release

Missing call to sync in UniV2LiquidityAmo#_sendTokensToRdpxV2Core

balance of reserves should be updated following any changes to the contract balances https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L160

usage of abi.encodePacked to compare string

hash collision could be possible due to how bytes are packed when using encodePacked.

function getReserveTokenInfo(
    string memory _token
  ) public view returns (address, uint256, string memory) {
    ReserveAsset memory asset = reserveAsset[reservesIndex[_token]];

    _validate(
      keccak256(abi.encodePacked(asset.tokenSymbol)) ==
        keccak256(abi.encodePacked(_token)),
      19
    );//@audit

    return (asset.tokenAddress, asset.tokenBalance, asset.tokenSymbol);
  }

Recommendation

use abi.encode instead of abi.encodePacked

Hardcoded deadline does not work

When interacting with AMM deadline happens to terminate a transaction which has been pending on the mempool which might have been executed on an unfavorable condition or timing. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L295

ISwapRouter.ExactInputSingleParams memory swap_params = ISwapRouter
      .ExactInputSingleParams(
        _tokenA,
        _tokenB,
        _fee_tier,
        address(this),
        2105300114, // Expiration: a long time from now@audit invalid deadline
        _amountAtoB,
        _amountOutMinimum,
        _sqrtPriceLimitX96
      );

Recommendation

Allow the Admin to pass a deadline deemed reasonable to swap.

Redundant checks should be removed

Within rdpxV2 there are some redundant checks which should be refactored. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L120

function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) {<@
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");//@audit this is a redundant check
  }

other Instances: https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1055 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1086

leftover tokenB from swaps could be swept to rdpxV2Core when reLPing.

When reLping part of tokenB is swapped for tokenA and the other part readded as Liquidity, within does swaps tokenB could have leftover from the swap with could accumulate and left stuck within the reLpContract. These leftovers could be transferred to rdpxV2Core as well. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L290

setBondDiscount should be constrained

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

function setBondDiscount(
    uint256 _bondDiscountFactor
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_bondDiscountFactor > 0, 3);
@>  bondDiscountFactor = _bondDiscountFactor;

    emit LogSetBondDiscountFactor(_bondDiscountFactor);
  }

bondDiscountFactor could be set to a value that could DoS bonding.

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

@>     _validate(bondDiscount < 100e8, 14); 
	..SNIP
}