Platform: Code4rena
Start Date: 21/08/2023
Pot Size: $125,000 USDC
Total HM: 26
Participants: 189
Period: 16 days
Judge: GalloDaSballo
Total Solo HM: 3
Id: 278
League: ETH
Rank: 143/189
Findings: 2
Award: $10.34
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0xrafaelnicolau
Also found by: 0x111, 0xCiphky, 0xMosh, 0xWaitress, 0xc0ffEE, 0xkazim, 0xnev, 0xvj, ABAIKUNANBAEV, Aymen0909, Baki, ElCid, HChang26, HHK, Inspex, Jorgect, Kow, Krace, KrisApostolov, LFGSecurity, MiniGlome, Nyx, QiuhaoLi, RED-LOTUS-REACH, Talfao, Toshii, Vagner, Viktor_Cortess, Yanchuan, _eperezok, asui, atrixs6, bart1e, bin2chen, carrotsmuggler, chaduke, chainsnake, deadrxsezzz, degensec, dethera, dimulski, dirk_y, ether_sky, gizzy, glcanvas, grearlake, gumgumzum, halden, hals, kodyvim, koo, ladboy233, lanrebayode77, max10afternoon, minhtrng, mussucal, nobody2018, peakbolt, pontifex, qbs, ravikiranweb3, rvierdiiev, said, tapir, ubermensch, volodya, wintermute, yashar, zaevlad, zzebra83
0.1468 USDC - $0.15
totalWethDelegated is not updated in all of the functionalities involving delegated WETH, which could cause, a malicious user to block all of the internal accounting of the protocol.
The variable totalWethDelegated is used to account for all of the WETH which is available to use in the bondWithDelegate mechanism. Every time someone intends to delegate their WETH to be used in the bonding process and gain some initiatives out of it, they can call addToDelegate which creates a new delegate position and adds the amount transferred to totalWethDelegated
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L941-L964
In the case where that specific user doesn't want to delegate their WETH anymore, they can call withdraw to withdraw their WETH out of the contract, updating the delegate position
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L975-L990
but nowhere in the withdraw, totalWethDelegated gets updated which means that an abuser can inflate totalWethDelegated as much as he wants by calling addToDelegate and then withdraw. The only way totalWethDelegated can lower it's value is only in the bonding with delegate mechanism, process which could fail since the value is inflated and there isn't that actual amount of WETH in the contract. Also because of totalWethDelegated being too big, all of the time sync would be called it will fail and revert because of the underflow that will happen here
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1002
when it tries to subtract totalWethDelegated from the balance that the contract has in WETH, which means that the whole UniV2LiquidityAmo.sol and UniV3LiquidityAmo.sol functionality would fail and revert, blocking important mechanisms used by the protocol, with their funds in UniswapV2 or V3.
Manual review
Every time withdraw is called, subtract also from the totalWethDelegated, the amount that the user provided, so in that way totalWethDelegated would be accurate to the WETH available in the contract for delegations.
Under/Overflow
#0 - c4-pre-sort
2023-09-07T08:01:23Z
bytes032 marked the issue as duplicate of #2186
#1 - c4-judge
2023-10-20T17:55:32Z
GalloDaSballo changed the severity to 2 (Med Risk)
#2 - c4-judge
2023-10-20T17:57:13Z
GalloDaSballo marked the issue as satisfactory
#3 - c4-judge
2023-10-20T18:01:09Z
GalloDaSballo removed the grade
#4 - c4-judge
2023-10-20T18:01:18Z
GalloDaSballo marked the issue as satisfactory
#5 - c4-judge
2023-10-21T07:38:54Z
GalloDaSballo changed the severity to 3 (High Risk)
🌟 Selected for report: Vagner
Also found by: 0Kage, 0xCiphky, 0xnev, ABAIKUNANBAEV, Aymen0909, Evo, KmanOfficial, MohammedRizwan, T1MOH, Viktor_Cortess, Yanchuan, ak1, alexzoid, bin2chen, codegpt, hals, ladboy233, mrudenko, nemveer, oakcobalt, peakbolt, pep7siup, qbs, said, savi0ur, tapir, wintermute, zaevlad, zzebra83
10.1883 USDC - $10.19
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L118-L133
The function sync is used in RdpxV2Core.sol to synchronize the balances of the reserve assets stored in the contract, but it is not called in all of the situations where swaps/providing/removing liquidity is made.
As you can see sync is called in UniV3LiquidityAmo.sol after _sendTokensToRdpxV2Core is called to occur for all of the balances change after modifying the liquidity in UniswapV3
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L361
but it is not called functions like collectFees where funds are directly transferred to RdpxV2Core.sol
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L118-L133
or in _sendTokensToRdpxV2Core in the UniV2LiquidityAmo.sol which transfers tokens to RdpxV2Core
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L157-L178
Because of that wrong assumptions can be taken in cases of lowerDepeg or upperDepeg, or any other functions that uses the balances of assets stored, if sync was not called before.
Manual review
Since you already call sync in _sendTokensToRdpxV2Core from UniV3LiquidityAmo.sol, call it in collectFees and _sendTokensToRdpxV2Core from UniV2LiquidityAmo.sol also to accounts for any balance changes, which will protect the protocol against errors.
Other
#0 - c4-pre-sort
2023-09-09T03:53:50Z
bytes032 marked the issue as duplicate of #798
#1 - c4-pre-sort
2023-09-09T04:06:58Z
bytes032 marked the issue as not a duplicate
#2 - c4-pre-sort
2023-09-09T04:07:02Z
bytes032 marked the issue as primary issue
#3 - bytes032
2023-09-09T04:11:06Z
Potentially impacts the following functions:
Edit: It’s actually not called in collectFee’s as well. I’m duplicating all of these under a single primary issue, because #269 mentions both.
#4 - c4-pre-sort
2023-09-11T11:57:22Z
bytes032 marked the issue as sufficient quality report
#5 - c4-sponsor
2023-09-25T17:26:00Z
witherblock (sponsor) confirmed
#6 - GalloDaSballo
2023-10-12T08:02:10Z
Seems like an opportunity for a higher exploit was missed by stopping at the broken sync
#7 - c4-judge
2023-10-15T18:11:07Z
GalloDaSballo marked issue #250 as primary and marked this issue as a duplicate of 250
#8 - c4-judge
2023-10-15T18:11:19Z
GalloDaSballo marked the issue as satisfactory
#9 - c4-judge
2023-10-15T18:14:22Z
GalloDaSballo changed the severity to 3 (High Risk)
#10 - c4-judge
2023-10-15T18:14:28Z
GalloDaSballo changed the severity to 2 (Med Risk)
#11 - c4-judge
2023-10-25T07:26:26Z
GalloDaSballo marked the issue as selected for report