Redacted Cartel contest - 0xbepresent's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L227 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L321 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L345

Vulnerability details

Impact

The compound() function helps to swap the gmxBaseReward (ether) for GMX tokens then the GMX tokens are deposited for pxGMX. In the swap function the amountOutMinimum helps to put the minimum value that is expected for the swap. The fixed amountOutMInimum assumes that the price can not be manipulated by different techniques so it is better to calculate the minimum accepted amount.

The amountOutMinimum should be calculated with the Uniswap SDK or an onchain price oracle as their documentation says:

For a real deployment, this value should be calculated using our SDK or an onchain price oracle - this helps protect against getting an unusually bad price for a trade due to a front running sandwich or another type of price manipulation.

Proof of Concept

The following lines use a fixed amountOutMinimum value:

• src/vaults/AutoPxGmx.sol#L227
• src/vaults/AutoPxGmx.sol#L321
• src/vaults/AutoPxGmx.sol#L345

Tools used

VisualStudio

Recommended Mitigation Steps

Calculates the amountOutMinimum using the Uniswap SDK, an onchain price oracle or a time weighted average price.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L386-L387

Vulnerability details

Impact

The user can lost his rewards if the reward token is removed from the producerTokens[producerToken].rewardTokens list. If the reward token is removed, the rewardToken length is going to be zero, the user rewards going to be zero and the for statement will never enter so the user will lost his rewards for this token and the global state rewards will save inaccurate information.

Proof of Concept

1. Add user pxGMX token an accrue his rewards
2. Remove rewardToken
3. The user won't get his rewards even if the reward token is added again

I did a test for this situation:

// test/PirexRewards.t.sol
function testClaimRewardsWithRemovedToken(
    uint32 secondsElapsed,
    uint8 multiplier,
    bool useETH
) external {
    vm.assume(secondsElapsed > 10);
    vm.assume(secondsElapsed < 365 days);
    vm.assume(multiplier != 0);
    vm.assume(multiplier < 10);
    address userX = testAccounts[0];
    uint256 gmxAmount = 100e18;

    _mintApproveGmx(gmxAmount, address(this), address(pirexGmx), gmxAmount);
    pirexGmx.depositGmx(gmxAmount, userX);

    assertEq(weth.balanceOf(userX), 0);

    vm.warp(block.timestamp + secondsElapsed);

    //
    // Add reward token and harvest rewards from Pirex contract
    //
    pirexRewards.addRewardToken(pxGmx, weth);
    pirexRewards.harvest();

    pirexRewards.userAccrue(pxGmx, userX);
    (, , uint256 globalRewardsBeforeClaimPxGmx) = _getGlobalState(
        pxGmx
    );
    (, , uint256 userRewardsBeforeClaimPxGmx) = pirexRewards
        .getUserState(pxGmx, userX);
    //
    // Sum of reward amounts that the user is entitled to
    //
    uint256 expectedClaimAmount = ((pirexRewards.getRewardState(
        pxGmx,
        weth
    ) * _calculateUserRewards(pxGmx, userX)) /
        _calculateGlobalRewards(pxGmx));

    //
    // Remove WETH from pxGMX
    //
    vm.expectEmit(true, false, false, true, address(pirexRewards));
    emit RemoveRewardToken(pxGmx, 0);
    pirexRewards.removeRewardToken(pxGmx, 0);

    // vm.expectEmit(true, false, false, false, address(pirexGmx));
    // emit ClaimUserReward(userX, address(0), 0, 0, 0);  // this emit is not called
    vm.expectEmit(true, true, false, true, address(pirexRewards));
    emit Claim(pxGmx, userX);
    pirexRewards.claim(pxGmx, userX);

    (, , uint256 globalRewardsAfterClaimPxGmx) = _getGlobalState(pxGmx);
    (, , uint256 userRewardsAfterClaimPxGmx) = pirexRewards.getUserState(pxGmx, userX);

    assertEq(
        globalRewardsBeforeClaimPxGmx - userRewardsBeforeClaimPxGmx,
        globalRewardsAfterClaimPxGmx
    );
    assertEq(0, userRewardsAfterClaimPxGmx);

    //
    // Owner weth balance has not changed. It is still zero.
    // userX has not received their rewards
    //
    assertEq(weth.balanceOf(userX), 0);
    assertTrue(expectedClaimAmount != weth.balanceOf(userX));

    // Add again the rewardToken
    pirexRewards.addRewardToken(pxGmx, weth);
    pirexRewards.harvest();
    
    // userX balance rewards is still zero
    pirexRewards.claim(pxGmx, userX);
    assertEq(weth.balanceOf(userX), 0);
}

Tools used

Foundry/VSCode

Recommended Mitigation Steps

Add a check if the reward token exists or not so the p.userStates[user].rewards is not zero if the reward token was removed:

...
...
ERC20[] memory rewardTokens = p.rewardTokens;
uint256 rLen = rewardTokens.length;

// Claim should be skipped and not reverted on zero global/user reward
if (globalRewards != 0 && userRewards != 0 && rLen != 0) {
    // Update global and user reward states to reflect the claim
    p.globalState.rewards = globalRewards - userRewards;
    p.userStates[user].rewards = 0;
    ...
    ...
}

1 - Use totalAssets() function.

Use the totalAssets() function in the corresponding contract.

Instances of this issue:

• src/vaults/AutoPxGmx.sol#L260
• src/vaults/AutoPxGlp.sol#L229

2 - Save the newAssets calculation in a variable and use it in the condition and next calculation

Save the newAssets calculation in a variable here src/vaults/AutoPxGmx.sol#L288 and use it in the 290 line. Basically it follows the same pattern from src/vaults/AutoPxGlp.sol#L250

Example:

uint256 newAssets = totalAssets() - assetsBeforeClaim;
if (newAssets != 0) {
	totalFee = (newAssets * platformFee) / FEE_DENOMINATOR;
}