Dopex - 0x111's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L975-L989 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L995-L1008

Vulnerability details

Impact

The sync between the reserve and the actual balance made in the sync() function will be wrong which will lead a wrong reserve balance for WETH which has an impact on the whole system

Proof of Concept

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

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L995-L1008

The sync() function makes sure the reserve balance and the actual balance match. For WETH, it substracts the current totalWethDelegated from the real balance and sets it as the tokenBalance in reserveAsset.

However, in the withdraw() function, when a user removes his WETH from delegation, the totalWethDelegated variable is not updated which leads to this delegation to still be counted in the sync() function when it's not delegated anymore. As it's never substracted it will always be removed from the reserveAsset.tokenBalance in the subsequent calls to the sync() function.

  function testAuditDelegateUpdate() public {

    uint256 userBeforeWethBalance = weth.balanceOf(address(this));
    (, uint256 wethReserveBefore, ) = rdpxV2Core.getReserveTokenInfo("WETH");
    uint256 wethCoreBalance = weth.balanceOf(address(rdpxV2Core));
    uint256 totalWethDelegated = rdpxV2Core.totalWethDelegated();
    console.log("User balance: %d", userBeforeWethBalance);
    console.log("WETH reserve: %d", wethReserveBefore);
    console.log("WETH core balance: %d", wethCoreBalance);
    console.log("totalWethDelegated: %d", totalWethDelegated);

    uint256 delegateId = rdpxV2Core.addToDelegate(10 * 1e18, 10 * 1e8);
    console.log("Delegate ID: %d", delegateId);

    userBeforeWethBalance = weth.balanceOf(address(this));
    (, wethReserveBefore, ) = rdpxV2Core.getReserveTokenInfo("WETH");
    wethCoreBalance = weth.balanceOf(address(rdpxV2Core));
    totalWethDelegated = rdpxV2Core.totalWethDelegated();
    console.log("User balance: %d", userBeforeWethBalance);
    console.log("WETH reserve: %d", wethReserveBefore);
    console.log("WETH core balance: %d", wethCoreBalance);
    console.log("totalWethDelegated: %d", totalWethDelegated);

    rdpxV2Core.withdraw(delegateId);

    userBeforeWethBalance = weth.balanceOf(address(this));
    (, wethReserveBefore, ) = rdpxV2Core.getReserveTokenInfo("WETH");
    wethCoreBalance = weth.balanceOf(address(rdpxV2Core));
    console.log("User balance: %d", userBeforeWethBalance);
    console.log("WETH reserve: %d", wethReserveBefore);
    console.log("WETH core balance: %d", wethCoreBalance);
    console.log("totalWethDelegated: %d", totalWethDelegated);

    rdpxV2Core.sync();

    userBeforeWethBalance = weth.balanceOf(address(this));
    (, wethReserveBefore, ) = rdpxV2Core.getReserveTokenInfo("WETH");
    wethCoreBalance = weth.balanceOf(address(rdpxV2Core));
    console.log("User balance: %d", userBeforeWethBalance);
    console.log("WETH reserve: %d", wethReserveBefore);
    console.log("WETH core balance: %d", wethCoreBalance);
    console.log("totalWethDelegated: %d", totalWethDelegated);

  }

In this example we call delegate() then immediately call withdraw() and sync(). The call to sync() will fail as an underflow error occurs because the WETH previously delegated keep being substracted from the balance even though they already have been withdrawn.

The underflow occurs as a the balance hits 0 but even in cases where the balance does not hit 0, it will create a disparity between the real balance and the reserveAsset.tokenBalance of WETH. As this tokenBalance is crucial for the whole system, I think this qualifies as a High Risk finding.

Tools Used

VSCode and Foundry

Recommended Mitigation Steps

Update the totalWethDelegated variable in the withdraw() function to make sure the sync() takes into account the delegations that are already withdrawn.

Assessed type

Other