Redacted Cartel contest - 8olidity's results

Lines of code

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

Vulnerability details

Impact

Per EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down. If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

Proof of Concept

The original implementation of previewWithdraw in solmate ERC4626 is:

    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
    }

It is rounding up,but the implementation of autopxglp.sol#previewWith rounds down

    function previewWithdraw(uint256 assets)
        public
        view
        override
        returns (uint256)
    {
        // Calculate shares based on the specified assets' proportion of the pool
        uint256 shares = convertToShares(assets);

        // Save 1 SLOAD
        uint256 _totalSupply = totalSupply;

        // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
        return
            (_totalSupply == 0 || _totalSupply - shares == 0)
                ? shares
                : (shares * FEE_DENOMINATOR) /
                    (FEE_DENOMINATOR - withdrawalPenalty);
    }

    function convertToShares(uint256 assets)
        public
        view
        virtual
        returns (uint256)
    {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());   // @audit 
    }

it might cause some intergration problem in the future that can lead to wide range of issues for both parties.

Tools Used

vscode

Recommended Mitigation Steps

Round up in previewWithdraw using mulDivUp and divWadUp

Lines of code

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

Vulnerability details

Impact

Most of the share based vault implementation will face this issue. The vault is based on the ERC4626 where the shares are calculated based on the deposit value. By depositing large amount as initial deposit, initial depositor can influence the future depositors value.

Proof of Concept

Shares are minted based on the deposit value. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L370-L404 Public vault is based on the ERC4626 where the shares are calculated based on the deposit value.

Reference: https://github.com/sherlock-audit/2022-08-sentiment-judging#issue-h-1-a-malicious-early-userattacker-can-manipulate-the-ltokens-pricepershare-to-take- an-unfair-share-of-future-users-deposits:~:text=Issue%20H%2D1%3A%20A%20malicious%20early%20user/attacker%20can%20manipulate%20the%20LToken%27s%20pricePerShare%20to% 20take%20an%20unfair%20share%20of%20future%20users%27%20deposits https://github.com/sherlock-audit/2022-10-astaria-judging#issue-m-26-first-erc4626-deposit-can-break-share-calculation

    function depositGmx(uint256 amount, address receiver)
        external
        nonReentrant
        returns (uint256 shares)
    {
        if (amount == 0) revert ZeroAmount();
        if (receiver == address(0)) revert ZeroAddress();

        // Handle compounding of rewards before deposit (arguments are not used by `beforeDeposit` hook)
        if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

        // Intake sender GMX
        gmx.safeTransferFrom(msg.sender, address(this), amount);

        // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
        (, uint256 assets, ) = PirexGmx(platform).depositGmx(
            amount,
            address(this)
        );

        // NOTE: Modified `convertToShares` logic to consider assets already being in the vault
        // and handle it by deducting the recently-deposited assets from the total
        uint256 supply = totalSupply;

        if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }


function convertToShares(uint256 assets) public view virtual returns (uint256) {
    uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

    return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}


function totalAssets() public view override returns (uint) {
    return asset.balanceOf(address(this)) + getBorrows() - getReserves();
}

Future depositors are forced for huge value of asset to deposit. It is not practically possible for all the users. This could directly affect on the attrition of users towards this system.

Tools Used

vscode

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L97 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L152-L158

Vulnerability details

Impact

When the contract is created, the approve operation will be performed on gmx

        gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max);
        gmx.safeApprove(_platform, type(uint256).max);

But the platform address can be updated

    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();

        platform = _platform;

        emit PlatformUpdated(_platform);
    }

Two issues are overlooked here

1. The original platform can still operate the gmx of the contract without revoking the authorization operation of the previous platform.
2. There is no approve() operation for the new platform. It will cause compound() and depositGmx() to fail because there is no approve operation for the new platform address

src/vaults/AutoPxGmx.sol:
  280              // Deposit entire GMX balance for pxGMX, increasing the asset/share amount
  281:             (, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
  282                  gmx.balanceOf(address(this)),

  384          // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
  385:         (, uint256 assets, ) = PirexGmx(platform).depositGmx(
  386              amount,

Proof of Concept

    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();

        platform = _platform;

        emit PlatformUpdated(_platform);
    }

Tools Used

vscode

Recommended Mitigation Steps

    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();
+      gmx.safeApprove(platform, 0);
        platform = _platform;
+      gmx.safeApprove(platform, type(uint256).max);
        emit PlatformUpdated(_platform);
    }

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L257

Vulnerability details

Impact

Consider the case where r.totalsupply is 0. When r.totalsupply is 0, it should return 0 directly, because there will be an error of dividing by 0.

Proof of Concept

    function _calculateRewards(bool isBaseReward, bool useGmx)
        internal
        view
        returns (uint256)
    {
        RewardTracker r;
        
        uint256 cumulativeRewardPerToken = r.cumulativeRewardPerToken() +
            ((blockReward * precision) / r.totalSupply()); // @audit

Tools Used

vscode

Recommended Mitigation Steps

if ( r.totalSupply() == 0) return 0;