Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded Q -> 2 from #425 [1698130939448] #2227

Closed
c4-judge opened this issue Oct 24, 2023 · 2 comments
Closed

Upgraded Q -> 2 from #425 [1698130939448] #2227

c4-judge opened this issue Oct 24, 2023 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value duplicate-269 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-judge
Copy link
Contributor

Judge has assessed an item in Issue #425 as 2 risk. The relevant finding follows:

  1. UniLiquidityAmo contracts doesn’t synchronize reserve balances of RdpxV2Core in some cases
    Impact
    Developer from Dopex said that “we keep the balances to check the health of dpxEth”. I talk about this balances: https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L61

/// @notice Array containg the reserve assets
ReserveAsset[] public reserveAsset;

/// @dev Struct containing the the rdpxV2Core reserve asset data
struct ReserveAsset {
address tokenAddress;
uint256 tokenBalance;
string tokenSymbol;
}
tokenBalance can differ from actual balance in this cases:

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L313-L322
function recoverERC20(
address tokenAddress,
uint256 tokenAmount
) external onlyRole(DEFAULT_ADMIN_ROLE) {
// Can only be triggered by owner or governance, not custodian
// Tokens are sent to the custodian, as a sort of safeguard
TransferHelper.safeTransfer(tokenAddress, rdpxV2Core, tokenAmount);

emit RecoveredERC20(tokenAddress, tokenAmount);

}
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133
function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
for (uint i = 0; i < positions_array.length; i++) {
Position memory current_position = positions_array[i];
INonfungiblePositionManager.CollectParams
memory collect_params = INonfungiblePositionManager.CollectParams(
current_position.token_id,
rdpxV2Core,
type(uint128).max,
type(uint128).max
);

  // Send to custodian address
  univ3_positions.collect(collect_params);
}

}
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178
function _sendTokensToRdpxV2Core() internal {
uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
address(this)
);
uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
address(this)
);
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(addresses.tokenA).safeTransfer(
addresses.rdpxV2Core,
tokenABalance
);
IERC20WithBurn(addresses.tokenB).safeTransfer(
addresses.rdpxV2Core,
tokenBBalance
);

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

}
Recommended Mitigation Steps
Call IRdpxV2Core(rdpxV2Core).sync() in all the referenced code blocks

@c4-judge c4-judge added the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Oct 24, 2023
@c4-judge
Copy link
Contributor Author

GalloDaSballo marked the issue as duplicate of #250

@c4-judge
Copy link
Contributor Author

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards duplicate-269 and removed duplicate-250 labels Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value duplicate-269 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

1 participant