code-423n4 / 2023-08-dopex-findings

3 stars 3 forks source link

Upgraded Q -> 2 from #1784 [1698218728214] #2231

Closed c4-judge closed 10 months ago

c4-judge commented 10 months ago

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

Low -2 UniV2LiquidityAmo.sol accounting might be temporarily out of sync In UniV2LiquidityAmo.sol, sync() is an external function that can be called by anyone to update the lpTokenBalance. And lpTokenBalance is modified in addLiquidity() and removeLiquidity().

The issue is addLiquidity() and removeLiquidity() won’t sync() the token balance before updating the token balance. In the case that lpTokenBalance is changed by either one of the function, but sync() is not called in time by users, and when the other function is modifying lpTokenBalance again the change in balance will be added or subtracted on an outdated number. if sync() is never called for a long time by users or anyone, these changes will continuously be added or substracted based on an out-of-sync lpTokenBalance.

//UniV2LiquidityAmo.sol function addLiquidity( uint256 tokenAAmount, uint256 tokenBAmount, uint256 tokenAAmountMin, uint256 tokenBAmountMin ) external onlyRole(DEFAULT_ADMIN_ROLE) returns (uint256 tokenAUsed, uint256 tokenBUsed, uint256 lpReceived) { ... // update LP token Balance lpTokenBalance += lpReceived; ...}

function removeLiquidity(
    uint256 lpAmount,
    uint256 tokenAAmountMin,
    uint256 tokenBAmountMin
)
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 tokenAReceived, uint256 tokenBReceived)
{

... lpTokenBalance -= lpAmount; ...} (https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L235) (https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L283)

function sync() external {
    lpTokenBalance = IERC20WithBurn(addresses.pair).balanceOf(
        address(this)
    );
}

Recommendations: Consider calling sync() inside addLiquidity() and removeLiquidity before updating lpTokenBalance.

c4-judge commented 10 months ago

GalloDaSballo marked the issue as duplicate of #269

c4-judge commented 10 months ago

GalloDaSballo marked the issue as satisfactory