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: 18/189
Findings: 5
Award: $1,031.85
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: klau5
Also found by: 0x3b, 0xCiphky, 0xDING99YA, 0xWaitress, 0xbranded, 0xc0ffEE, 0xklh, 0xsurena, 0xvj, ABA, AkshaySrivastav, Anirruth, Aymen0909, Baki, Blockian, BugzyVonBuggernaut, DanielArmstrong, Evo, GangsOfBrahmin, HChang26, Inspex, Jiamin, Juntao, Kow, Krace, KrisApostolov, LFGSecurity, LokiThe5th, Mike_Bello90, Norah, Nyx, QiuhaoLi, RED-LOTUS-REACH, SBSecurity, Snow24, SpicyMeatball, T1MOH, Tendency, Toshii, Udsen, Yanchuan, __141345__, ak1, asui, auditsea, ayden, bart1e, bin2chen, blutorque, carrotsmuggler, chaduke, chainsnake, circlelooper, clash, codegpt, crunch, degensec, dirk_y, ge6a, gjaldon, grearlake, jasonxiale, juancito, ke1caM, kodyvim, kutugu, ladboy233, lanrebayode77, mahdikarimi, max10afternoon, mert_eren, nirlin, nobody2018, oakcobalt, parsely, peakbolt, pks_, pontifex, ravikiranweb3, rokinot, rvierdiiev, said, savi0ur, sces60107, sh1v, sl1, spidy730, tapir, tnquanghuy0512, ubermensch, visualbits, volodya, wintermute
0.0098 USDC - $0.01
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L764-L783 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205
Settle function in rdpxv2Core.sol
can be dossed by sending the 1wei of weth into the perpetualAtlanticValutLP.sol
When the settle function is called, it calls the subtractLoss
function on Lp vault, that balances out the total collateral by subtracting the loss accured from it.
But if we take a look at the subtract loss function we can see a hard check for the balance:
function subtractLoss(uint256 loss) public onlyPerpVault { require( collateral.balanceOf(address(this)) == _totalCollateral - loss, "Not enough collateral was sent out" ); _totalCollateral -= loss; }
So if anyone send 1wei of token into this contract it will be dossed as the hard check for the balance will fail
I made the following change in the test case for the settle function where address(1) sends exactly 1wei of token in lp vault right before the settle function is called.
function testSettle() public { rdpxV2Core.bond(5 * 1e18, 0, address(this)); rdpxV2Core.bond(1 * 1e18, 0, address(this)); (, uint256 rdpxReserve1, ) = rdpxV2Core.getReserveTokenInfo("RDPX"); (, uint256 wethReserve1, ) = rdpxV2Core.getReserveTokenInfo("WETH"); vault.addToContractWhitelist(address(rdpxV2Core)); uint256[] memory _ids = new uint256[](1); // test OTM options vm.expectRevert( abi.encodeWithSelector( PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 7 ) ); + deal(address(weth),address(1), 100e18); + vm.startPrank(address(1)); + // transfer some weth token to the lp vault + weth.transfer(address(vault), 1); + vm.stopPrank(); + rdpxV2Core.settle(_ids); // settle invalid option id _ids[0] = 1; vm.expectRevert( abi.encodeWithSelector( PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 7 ) ); rdpxV2Core.settle(_ids); (uint256 strike1, uint256 amount1, ) = vault.optionPositions(0); // test ITM options _ids[0] = 0; rdpxPriceOracle.updateRdpxPrice(1e7); rdpxV2Core.settle(_ids); (, uint256 rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX"); (, uint256 wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH"); assertEq(rdpxReserve1 - amount1, rdpxReserve2); assertEq(wethReserve1 + ((amount1 * strike1) / 1e8), wethReserve2); rdpxV2Core.bond(10 * 1e18, 0, address(this)); rdpxV2Core.bond(1 * 1e18, 0, address(this)); (, rdpxReserve1, ) = rdpxV2Core.getReserveTokenInfo("RDPX"); (, wethReserve1, ) = rdpxV2Core.getReserveTokenInfo("WETH"); // test for multiple options with settled option id _ids = new uint256[](3); _ids[0] = 0; _ids[1] = 2; _ids[2] = 3; vm.expectRevert( abi.encodeWithSelector( PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 7 ) ); rdpxV2Core.settle(_ids); // settle multiple options at different strikes _ids[0] = 1; _ids[1] = 2; _ids[2] = 3; (strike1, amount1, ) = vault.optionPositions(1); (uint256 strike2, uint256 amount2, ) = vault.optionPositions(2); (uint256 strike3, uint256 amount3, ) = vault.optionPositions(3); rdpxPriceOracle.updateRdpxPrice(1e6); rdpxV2Core.settle(_ids); (, rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX"); (, wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH"); assertEq(rdpxReserve1 - amount1 - amount2 - amount3, rdpxReserve2); assertEq( wethReserve1 + ((amount1 * strike1) / 1e8) + ((amount2 * strike2) / 1e8) + ((amount3 * strike3) / 1e8), wethReserve2 ); }
This function fails with the following error output:
Failing tests: Encountered 1 failing test in tests/perp-vault/Unit.t.sol:Unit [FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 913849)
Which is the error thrown in subtract loss.
Admin can reverse the doss by calling the emergency withdraw and than sending back the exact required tokens and that is hassle and someone can again DOS by simply sending 1wei token or build a bot for it or even frontrunning each settle transaction. So it essentially becomes a permanent loss.
Dreaming about code after finding nothing for a while
chnage the subtractLoss()
as following:
function subtractLoss(uint256 loss) public onlyPerpVault { require( - collateral.balanceOf(address(this)) == _totalCollateral - loss, + collateral.balanceOf(address(this)) >= _totalCollateral - loss, "Not enough collateral was sent out" ); _totalCollateral -= loss; }
DoS
#0 - c4-pre-sort
2023-09-09T09:54:08Z
bytes032 marked the issue as duplicate of #619
#1 - c4-pre-sort
2023-09-11T16:14:19Z
bytes032 marked the issue as sufficient quality report
#2 - c4-judge
2023-10-21T07:15:59Z
GalloDaSballo marked the issue as satisfactory
🌟 Selected for report: 0xTheC0der
Also found by: 0Kage, 0xDING99YA, 0xHelium, 0xbranded, 836541, ABA, Kow, QiuhaoLi, SpicyMeatball, T1MOH, __141345__, alexfilippov314, ayden, bart1e, bin2chen, chaduke, degensec, jasonxiale, joaovwfreire, nirlin, peakbolt, pep7siup, rvierdiiev, tnquanghuy0512
15.9268 USDC - $15.93
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L237-L241 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L405-L459 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L462-L524
perpetualAtlanticVault.sol
functionality is dependent upon the funding duration which can be changed by the admin and can lead to bricking the whole calculations in the perpetualAtlanticVault.sol
Initially funding duration is set to seven days which can be changed using the following function:
function updateFundingDuration( uint256 _fundingDuration ) external onlyRole(DEFAULT_ADMIN_ROLE) { fundingDuration = _fundingDuration; }
And one of the important function that is dependent upon the funding duration is:
/// @inheritdoc IPerpetualAtlanticVault function nextFundingPaymentTimestamp() public view returns (uint256 timestamp) { return genesis + (latestFundingPaymentPointer * fundingDuration); }
Which is used overall in the contract to calculate start times and end times and while paying the fundings in drip vested manner
Lets consider the following function and it will explain the overall problem in other functions too:
function updateFunding() public { // what if even we don't update this funding payment pointer, will it make any difference at all? updateFundingPaymentPointer(); uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer]; uint256 startTime = lastUpdateTime == 0 ? (nextFundingPaymentTimestamp() - fundingDuration) : lastUpdateTime; lastUpdateTime = block.timestamp; collateralToken.safeTransfer( addresses.perpetualAtlanticVaultLP, (currentFundingRate * (block.timestamp - startTime)) / 1e18 ); IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds( (currentFundingRate * (block.timestamp - startTime)) / 1e18 ); emit FundingPaid( msg.sender, ((currentFundingRate * (block.timestamp - startTime)) / 1e18), latestFundingPaymentPointer ); }
lets say initially the funding duration was 7 days for first 4 epochs. And after 4 epochs admin/protcol governance decide that the ideal parameter should be 5 days. What would happen, lets see
consider the following code snippet :
uint256 startTime = lastUpdateTime == 0 ? (nextFundingPaymentTimestamp() - fundingDuration) : lastUpdateTime; lastUpdateTime = block.timestamp;
lets say the lastUpdateTime == 0
And 4 epochs have passed, which means 4 weeks have gone by after the epoch, 35 days, if genesis timestamp is at 7th day after deployment, so 28 days after genesis. But because new funding duration is 5 days the start time becomes:
startTime = (genesis (lets say initially it was the 7th day) + (pointer ( which is 4) * fundingDuration(which is 5)) - fundingDuration
so startTime = genesis + 20 days.
So our startTime decreases 8 days and collide with the previous star time stamps, for which the funding could be already paid and will result in double payment.
Similarly funding could be skipped for certain time in between if the funding duration is increased instead of decreased. And whole drip vesting is dependent upon this logic.
Also funding duration plays important part in premium calculation to calculate the time till expiry. That will be also bricket
uint256 timeToExpiry = nextFundingPaymentTimestamp() - (genesis + ((latestFundingPaymentPointer - 1) * fundingDuration));
Manual review
Two solution here, either make the funding duration constant at the time of deployment, if not the whole contract will need a refactor here
Math
#0 - c4-pre-sort
2023-09-08T06:23:39Z
bytes032 marked the issue as duplicate of #980
#1 - c4-pre-sort
2023-09-11T08:19:59Z
bytes032 marked the issue as sufficient quality report
#2 - c4-judge
2023-10-20T11:12:26Z
GalloDaSballo marked the issue as satisfactory
🌟 Selected for report: said
Also found by: 0xWaitress, Nyx, RED-LOTUS-REACH, Tendency, __141345__, carrotsmuggler, nirlin, peakbolt, qpzm, wallstreetvilkas
158.2271 USDC - $158.23
In bond cost function the pointer is not updated to latest pointer which leads to wrong calculation of expiry timing. Expiring timing is the main key factor to calculate the premium, wrong expiry time leads to wrong premium hence, loss for either user or the protocol.
Have a look at the following function:
function calculateBondCost( uint256 _amount, uint256 _rdpxBondId ) public view returns (uint256 rdpxRequired, uint256 wethRequired) { uint256 rdpxPrice = getRdpxPrice(); if (_rdpxBondId == 0) { // if bond id is zero it is non decaying bonds we talking about // non decaying bonds, we passed in the amount and the bond Id uint256 bondDiscount = (bondDiscountFactor * Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) * 1e2) / (1e9); // 1e8 precision // (bondDiscoundFactor * squaretootof(rdpxReserve) * 100) / (squaretootof(1e18)) _validate(bondDiscount < 100e8, 14); // rdpx is 25% so we need to make the 25% discount on the value it creates lets say the amount is 100e8 // so its 25% is 25, so discount should be calculated on 25 rdpxRequired = ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount * DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2); wethRequired =((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /(DEFAULT_PRECISION * 1e2); } else { // if rdpx decaying bonds are being used there is no discount rdpxRequired = (RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2); wethRequired = (ETH_RATIO_PERCENTAGE * _amount) / (DEFAULT_PRECISION * 1e2); } uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault) .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price // @note - update the pointer first. uint256 timeToExpiry = IPerpetualAtlanticVault( addresses.perpetualAtlanticVault ).nextFundingPaymentTimestamp() - block.timestamp; if (putOptionsRequired) { wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault) .calculatePremium(strike, rdpxRequired, timeToExpiry, 0); } }
Here we can see expiry time is calculated at:
uint256 timeToExpiry = IPerpetualAtlanticVault( addresses.perpetualAtlanticVault ).nextFundingPaymentTimestamp() - block.timestamp;
And formula for nextFundingPaymentTimestamp()
given in the perpetualAtlanticVault.sol
as:
genesis + (latestFundingPaymentPointer * fundingDuration)
But latestFundingPaymentPointer
could be for the behind the latest epoch, which is why we always call the function updateFundingPaymentPointer()
before making any critical calculation.
As we can see when we calculate the premium in atlantic vault in calculateFunding function first the pointer is updated by calling the updateFundingPaymentPointer()
as in following function from atlantic vault:
function calculateFunding( uint256[] memory strikes ) external nonReentrant returns (uint256 fundingAmount) { _whenNotPaused(); _isEligibleSender(); updateFundingPaymentPointer(); .....> skipped the code in between uint256 timeToExpiry = nextFundingPaymentTimestamp() - (genesis + ((latestFundingPaymentPointer - 1) * fundingDuration)); uint256 premium = calculatePremium( strike, amount, timeToExpiry, getUnderlyingPrice() ); latestFundingPerStrike[strike] = latestFundingPaymentPointer; fundingAmount += premium; // Record number of options that funding payments were accounted for, for this epoch // this accounting is only done for the epoch that start after the genesis fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount; // record the number of options funding has been accounted for the epoch and strike fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][ strike ] += amount; // Record total funding for this epoch // This does not need to be done in purchase() since it's already accounted for using `addProceeds()` totalFundingForEpoch[latestFundingPaymentPointer] += premium; emit CalculateFunding( msg.sender, amount, strike, premium, latestFundingPaymentPointer ); } }
But in our case in the rdpxV2Core
step of updating pointer is skipped.
Caffeine, neurons, vscode and alot of frustration.
This could have been prevented by updating the pointer in either of three functions in first line :
bond()
delgateBond()
calculateBondCost
If we choose first two we have to call the function in both the function. But putting it in last function compensate above two too.
call the updateFundingPaymentPointer
in the calculateBondCost()
and modify the code as following:
function calculateBondCost( uint256 _amount, uint256 _rdpxBondId ) public view returns (uint256 rdpxRequired, uint256 wethRequired) { + vault.updateFundingPaymentPointer() uint256 rdpxPrice = getRdpxPrice(); if (_rdpxBondId == 0) { // if bond id is zero it is non decaying bonds we talking about // non decaying bonds, we passed in the amount and the bond Id uint256 bondDiscount = (bondDiscountFactor * Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) * 1e2) / (1e9); // 1e8 precision // (bondDiscoundFactor * squaretootof(rdpxReserve) * 100) / (squaretootof(1e18)) _validate(bondDiscount < 100e8, 14); // rdpx is 25% so we need to make the 25% discount on the value it creates lets say the amount is 100e8 // so its 25% is 25, so discount should be calculated on 25 rdpxRequired = ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount * DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2); wethRequired =((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /(DEFAULT_PRECISION * 1e2); } else { // if rdpx decaying bonds are being used there is no discount rdpxRequired = (RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2); wethRequired = (ETH_RATIO_PERCENTAGE * _amount) / (DEFAULT_PRECISION * 1e2); } uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault) .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price // @note - update the pointer first. uint256 timeToExpiry = IPerpetualAtlanticVault( addresses.perpetualAtlanticVault ).nextFundingPaymentTimestamp() - block.timestamp; if (putOptionsRequired) { wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault) .calculatePremium(strike, rdpxRequired, timeToExpiry, 0); } }
Math
#0 - c4-pre-sort
2023-09-10T12:54:28Z
bytes032 marked the issue as duplicate of #237
#1 - c4-pre-sort
2023-09-12T06:09:05Z
bytes032 marked the issue as sufficient quality report
#2 - c4-pre-sort
2023-09-14T09:35:05Z
bytes032 marked the issue as duplicate of #761
#3 - c4-judge
2023-10-20T11:57:27Z
GalloDaSballo marked the issue as satisfactory
#4 - c4-judge
2023-10-21T07:54:55Z
GalloDaSballo changed the severity to 2 (Med Risk)
🌟 Selected for report: degensec
Also found by: 0x3b, 0xnev, HChang26, KmanOfficial, QiuhaoLi, T1MOH, WoolCentaur, Yanchuan, ayden, bart1e, jasonxiale, kutugu, mert_eren, nirlin, peakbolt, peanuts, pep7siup, qpzm, tapir, ubermensch, wintermute
24.8267 USDC - $24.83
Postion of funds can be list if some of thefunds haven't been deposited to the underlying Uniswap pools. There's always a chance of such event since Uniswap pools take balanced token amounts
Relp contract calls the addLiquidity()
function on the uniswap v2 pool, if we have a look at this function in uniswap v2 router, it calls another internal function '_addLiquidity()' which have the following code:
function _addLiquidity( address tokenA, address tokenB, uint amountADesired, uint amountBDesired, uint amountAMin, uint amountBMin ) internal virtual returns (uint amountA, uint amountB) { // create the pair if it doesn't exist yet if (IUniswapV2Factory(factory).getPair(tokenA, tokenB) == address(0)) { IUniswapV2Factory(factory).createPair(tokenA, tokenB); } (uint reserveA, uint reserveB) = UniswapV2Library.getReserves(factory, tokenA, tokenB); if (reserveA == 0 && reserveB == 0) { (amountA, amountB) = (amountADesired, amountBDesired); } else { uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB); if (amountBOptimal <= amountBDesired) { require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT'); (amountA, amountB) = (amountADesired, amountBOptimal); } else { uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA); assert(amountAOptimal <= amountADesired); require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT'); (amountA, amountB) = (amountAOptimal, amountBDesired); } } }
It calculates the balanced amount of amountA and amountB to be transferred from the user internally not what user wanted to send, so there is always a chance that due to slippage or some calculation, some amount of tokenA and tokenB is left in the contract which can accumulate overtime as relp can be called very often if it is toggle on.
Not token A in not the problem here due to following snippet in Relp:
IERC20WithBurn(addresses.tokenA).safeTransfer( addresses.rdpxV2Core, IERC20WithBurn(addresses.tokenA).balanceOf(address(this)) );
token A is sent back to core. But the tokenB go no where remainning stuck forever in contract. And unlike other contracts, this contract have no emergency withdraw either .
For more reference Jeiwan submission in good entry:
Inspired from Jeiwan submission in good entry contest which was heavily based upon the uniswap.
Add a lines to send back the tokenB too like tokenA
IERC20WithBurn(addresses.tokenB).safeTransfer( addresses.rdpxV2Core, IERC20WithBurn(addresses.tokenB).balanceOf(address(this)) );
Token-Transfer
#0 - c4-pre-sort
2023-09-07T12:41:00Z
bytes032 marked the issue as duplicate of #1259
#1 - c4-pre-sort
2023-09-07T12:41:24Z
bytes032 marked the issue as not a duplicate
#2 - c4-pre-sort
2023-09-07T12:46:44Z
bytes032 marked the issue as duplicate of #1286
#3 - c4-pre-sort
2023-09-11T07:50:55Z
bytes032 marked the issue as not a duplicate
#4 - c4-pre-sort
2023-09-11T15:37:48Z
bytes032 marked the issue as sufficient quality report
#5 - c4-pre-sort
2023-09-13T11:37:21Z
bytes032 marked the issue as duplicate of #1286
#6 - c4-judge
2023-10-10T17:52:40Z
GalloDaSballo changed the severity to 2 (Med Risk)
#7 - c4-judge
2023-10-18T12:13:16Z
GalloDaSballo marked the issue as satisfactory
🌟 Selected for report: catellatech
Also found by: 0xSmartContract, 0xnev, K42, LokiThe5th, RED-LOTUS-REACH, Sathish9098, __141345__, bitsurfer, hunter_w3b, mark0v, nirlin, rjs, rokinot
832.8493 USDC - $832.85
Dopex is an advanced decentralised option trading solution. with this audit dopex is going to introduce a new coin that is pegged to the value of eth backed by the reserves assets of Ethereum and rdpx. Bonding will happen via options and will also lead to minting out of the stable coin.
To maintain the peg, dopex have meny noval mechanims, such as amo, Relp and over collateralizing and de collateralizing techniques.
Along with these there are vaults that will be managing the Lp and their funding via the premiums paid. For the first epoch option buyer will himself pay the premium and later on all the premium will be paid by protocol in start of each epoch.
The key contracts of the protocol for this Audit are:
RdpxV2Core: This function is responsible for performing all the functionalities by using other contracts. It is the entry point for most of things.
Rdpx Decaying Bond Contract: Only admin can mint these bonds and are source of compensation for the users for loss accured on the protocol.
PerpetualAtlanticVault Contract: Contract which is used to pay all the premiums and fundings to the lp's.
PerpetualAtlanticVaultlp Contract: Main entry point for the liquidity provider and is also used by the vault to keep record of funds and assets availible to be used.
AMOContract: Two type of AMO are being used, this concept is inspired from the FRAX. Use to add and remove liquidity to the pools
ReLP Contract: This contract simply removes the eth liquidity and swap back it into dpx to increase the its value and value of backing reserves.
Existing Patterns: The protocol uses standard Solidity and Ethereum patterns. It uses the ERC721
standards for most part and Accesibility
pattern to grant different roles and also there is whitelisting
and pausing
mechanism too.
Protocol is also partially based on ERC4626 standard too but does not follow it completely and it is intentional.
I took the top to down approach for this audit, so general flow is as follow
1. rdpxV2Core.sol 2. perpetualAtlanticVault.sol 3. perpetualAtlanticVaultLP.sol 3. Amo and Relp 4. RdpxDecayingBonds.sol
Vaults, Amo and ReLp were reviewed side by side as they have close relationship with each other and balance out each other functionalities or build on top of each other and core interact with each of them. But decaying bonds contract can be reviewed separately.
I would say the codebase quality is good but can be improved, there are checks in place to handle different roles, each standard is ensured to be followed. And if something is not fully being followed that have been informed. But still it can be improved in following areas
Codebase Quality Categories | Comments |
---|---|
Unit Testing | Code base is well-tested but there is still place to add fuzz testing and invariant testing and foundry have now solid support for those, I would say best in class. |
Code Comments | Overall comments in code base were not enough, more commenting can be done to more specifically describe specifically mathematical calculation. At many point if there were more detail commenting and natspec it would have been bit easy for the auditor and less question would have been asked from sponser, saving time for both. Specifically perpetual vault needs more comments as its flow is hard to understand |
Documentation | Documentation was to the point but not detailed enough, such novelty requires more documentation than one notion page, that can be improved and I am sure will be done as previous products have full proof documentation on dopex website. |
Organization | Code orgranization was great for core, decaying bonds, amo's and Relp. A bit of improvement is needed in vault and lp vault, specifically in vault every thing was all over the place. If some one is not familiar with the flow of function he will remain lost. |
It is important to note that there are many previliged function in the system that if went rogue can cause significant problems for the protocol, for example roles for minter, fee setters, deployer and other roles granted using access control. But the bigger threat is that in every contract except for ReLP contract there is an emergency withdraw function that can be easily misused at some point and is a very big threat to the protocol.
Emergency withdraw function for either ERC20 or ERC721 or for both is present in following contract:
Theses function may seem necessary in some cases but are constant risk to the protocol. Even for multisig wallets keys keep getting compromised.
The whole system is overall very dependent upon the rdpx, dpx, and weth pools on uniswap but while anaysing these on a tool developed by chaos lab here:
https://community.chaoslabs.xyz/uniswap/twap
It can be seen that rdpx/weth pool on both uniswap v2 and v3 can be manipulated for a very less cost, and the highly dependence of dopex on these pool is an innate risk for the protocol.
Lets have a look here on both uniswap v2 and v3
Uniswap v2 analysis Uniswap V3 analysis
And even worse for dpx/weth pool as the manipulating the price by 10% only cost around $2000. And exhaustion cost is also way lower than v3 pool.
And dopex is highly dependence on these. so my recommendation is first focus on increasing the liquidity of these pools and making than more manipulation proof will be a good move.
Overall dopex came up with a very strong solution with some weak points in commenting, docs and some centralisation risk. But the approach used for the core working of protocol is really solid and up to the industry standard and fixing above recommendation will make it even more robust.
20 hours
#0 - c4-pre-sort
2023-09-15T15:52:58Z
bytes032 marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-09-15T15:54:42Z
bytes032 marked the issue as high quality report
#2 - c4-judge
2023-10-20T09:53:42Z
GalloDaSballo marked the issue as grade-a