Redacted Cartel contest - adriro's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L190 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L212

Vulnerability details

The function previewWithdraw is overridden in the AutoPxGmx and AutoPxGlp contracts to account for penalty fees while exiting the vaults. This happens in line 212 of the AutoPxGmx contract and similarly in line 190 of the AutoPxGlp contract:

return
    (_totalSupply == 0 || _totalSupply - shares == 0)
        ? shares
        : (shares * FEE_DENOMINATOR) /
            (FEE_DENOMINATOR - withdrawalPenalty);

Here, the integer division will round down, while the correct behavior should be to round up in favor of the protocol.

Impact

This issue would allow a user to withdraw tokens without paying penalties, which should not represent a big impact in a realistic scenario since it would need to involve handling extremely low quantities of assets/shares or withdrawing assets in batches of a relatively small amount (which should be discouraged by gas costs).

During each call to withdraw, if the division is not exact, the protocol will be losing 1 share of penalties due to rounding.

The correct way should be to round up the division, following the base ERC4626 implementation of the previewWithdraw function which uses mulDivUp.

Proof of Concept

The following test demonstrates the issue. In this case the user mints 10 shares and then withdraws 1 token by burning a single share without any penalty.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "forge-std/Test.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";
import {AutoPxGmx} from "src/vaults/AutoPxGmx.sol";
import {PirexGmx} from "src/PirexGmx.sol";
import {Helper} from "./Helper.sol";

contract AuditTest is Helper {
    function test_AutoPxGmx_WithdrawWrongRounding() public {        
        address user = testAccounts[0];

        uint256 userBalance = 10;
        _mintPx(user, userBalance, true);
                
        // Approve contract
        vm.prank(user);
        pxGmx.approve(address(autoPxGmx), type(uint256).max);
        
        // User deposits balance, he gets 10 shares
        vm.prank(user);
        autoPxGmx.deposit(userBalance, user);        
        assertEq(autoPxGmx.balanceOf(user), userBalance);
        
        vm.prank(user);
        uint256 shares = autoPxGmx.previewWithdraw(1);
        
        // Only 1 share needed to withdraw 1 token
        assertEq(shares, 1);
        
        vm.prank(user);
        autoPxGmx.withdraw(1, user, user);
        
        // User has withdrawn 1 token and still has 9 shares 
        assertEq(pxGmx.balanceOf(user), 1);
        assertEq(autoPxGmx.balanceOf(user), 9);   
    }
}

Recommended Mitigation Steps

Change the penalty calculation in the previewWithdraw function to round up:

return
    (_totalSupply == 0 || _totalSupply - shares == 0)
        ? shares
        : shares.mulDivUp(FEE_DENOMINATOR, FEE_DENOMINATOR - withdrawalPenalty);

Disable initializers in implementation contracts of PirexRewards

Even though the contract doesn't expose any delegatecall or way to call selfdestruct (it uses transparent proxies instead of UUPS), consider adding a constructor to disable initializers in the implementation contract of PirexRewards.

constructor() {
    _disableInitializers();
}

Call to super instead of duplicating code in transfer and transferFrom functions of PxERC20 contract

Both functions have the same code as their respective base ERC20 implementation with an added code to accrue rewards. Call the base implementation instead of duplicating all the code. For example:

function transfer(address to, uint256 amount)
    public
    override
    returns (bool)
{
    bool result = super.transfer(to, amount);
    
    // Accrue rewards for sender, up to their current balance and kick off accrual for receiver
    pirexRewards.userAccrue(this, msg.sender);
    pirexRewards.userAccrue(this, to);    

    return result;
}

Incorrect comment in AutoPxGlp constructor

The comment in line 86 reads:

// Approve the Uniswap V3 router to manage our base reward (inbound swap token)

However this seems to be a badly copy pasted comment from the AutoPxGmx contract. The approval is to the PirexGmx contract in order to compound the rewards.

Call to super instead of duplicating code in withdraw and redeem functions of AutoPxGmx contract

Both functions have the same code as their respective base ERC4626 implementation with a prepended code to compound the vault. Call the base implementation instead of duplicating all the code. For example:

function withdraw(
    uint256 assets,
    address receiver,
    address owner
) public override returns (uint256 shares) {
    // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
    compound(poolFee, 1, 0, true);
    
    shares = super.withdraw(assets, receiver, owner);
}

Unneeded zero address check in PirexGmx.depositGlp function

The token parameter is checked to guard against the zero address, but then the same parameter is validated using a list of whitelisted addresses, which should also cover the zero address check.

Use immutable variable for stakedGlp token in AutoPxGlp contract

The implementation of depositFsGlp fetches the stakedGlp token in every call using the PirexGmx contract (platform). Since this is a fixed token from the GMX protocol, consider using an immutable variable defined at construction time.

Use an infinite approve for the stakedGlp token in the AutoPxGlp contract

The function depositFsGlp will approve the spending for the given amount in every call. Consider adding an infinite approval during contract construction, similar to how it's done with the gmxBaseReward token.

Duplicated call to totalAssets() in compound function of AutoPxGmx contract

The function fetches the total amount of assets in line 288 (in the if condition) and does the same calculation in line 290 (asset.balanceOf(address(this)) - assetsBeforeClaim).

pxGmx token can be an immutable variable in the PxGmxReward contract

The pxGmx variable is defined at construction and can't be modified by the contract. Consider using an immutable variable to save gas.