Redacted Cartel contest - JohnSmith's results

Lines of code

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

Vulnerability details

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

Impact

Users may suffer loss of a significant portion of the funds they deposited to the Vault.

Proof of Concept

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the PxGLP, and get 1 wei of shares.

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

All external deposit functions lead to this _deposit()

src/vaults/AutoPxGlp.sol
304:     function _deposit(uint256 assets, address receiver)
305:         internal
306:         returns (uint256 shares)
307:     {
308:         // Check for rounding error since we round down in previewDeposit.
309:         uint256 supply = totalSupply;
310: 
311:         if (
312:             (shares = supply == 0
313:                 ? assets
314:                 : assets.mulDivDown(supply, totalAssets() - assets)) == 0
315:         ) revert ZeroShares();
316: 
317:         _mint(receiver, shares);
318: 
319:         emit Deposit(msg.sender, receiver, assets, shares);
320: 
321:         afterDeposit(receiver, assets, shares);
322:     }

First user will get 1 wei of shares for 1 wei of assets because of L313 After manipulations next users will get only 1 wei of shares for 19999e18 of asset because of assets.mulDivDown(supply, totalAssets() - assets)

Tools Used

Manual Review

Recommended Mitigation Steps

• Consider requiring a minimal amount of share tokens to be minted for the first minter, and send part of the initial mints as a permanent reserve to the DAO/treasury/deployer so that the pricePerShare can be more resistant to manipulation.
• An alternative is to require only the first depositor to freeze big enough initial amount of liquidity. This approach has been used long enough by various projects, for example in Uniswap V2: Uniswap/UniswapV2Pair.sol#L119-L121
• Maybe add a parameter minSharesOut, where user can set minimum amount of shares he expects to acquire.
• See how BalancerV2 and UniswapV2 did it. Some MINIMUM amount of pool tokens get burnt when the first mint happens
• You can also go BentoBox route of allowing total supply to go zero but not anywhere between 0 and MINIMUM.

[L-01] Disable initializers

As per OpenZeppelin recommendation Initializing the Implementation Contract

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

Findings

src/PirexRewards.sol
15: contract PirexRewards is OwnableUpgradeable {

Mitigation

To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

[L-02] safeApprove() is deprecated

safeApprove is deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). Some tokens do not allow approving an amount M > 0 when an existing amount N > 0 is already approved. If GMX adds to whitelisted such token functions depositGlp will revert

Findings

src/PirexGmx.sol
507:             t.safeApprove(glpManager, tokenAmount);//@audit check for approve(0) for whitelisted/not whitelisted coins

src/vaults/AutoPxGlp.sol
391:             erc20Token.safeApprove(platform, tokenAmount);

[G-01] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

See this issue for a detail description of the issue Constant expressions are left as expressions, not constants. It is re-calculated each time it is in use. Each usage of such "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

Findings

src/PxERC20.sol
9:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
10:     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

Mitigation

Use immutable instead of constant

[G-02]. x += y costs more gas than x = x + y for state variables

x += y costs more than x = x + y same as x -= y

Findings

src/PirexRewards.sol
361:         producerState.rewardStates[rewardTokens[i]] += r;

src/PxERC20.sol
85:         balanceOf[msg.sender] -= amount;

90:         balanceOf[to] += amount;

119:        balanceOf[from] -= amount;

124:        balanceOf[to] += amount;

src/vaults/PxGmxReward.sol
95:             rewardState += rewardAmount;

Mitigation

Replace x += y and x -= y with x = x + y and x = x - y.

[G-03] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage :

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. It is only beneficial to use reduced-size arguments if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation. When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values.

Findings

src/vaults/
AutoPxGmx.sol
26:     uint24 public poolFee = 3000;

39:     event PoolFeeUpdated(uint24 _poolFee);

46:         uint24 fee,

104:     function setPoolFee(uint24 _poolFee) external onlyOwner {


src/PirexFees.sol
20:     uint8 public constant FEE_PERCENT_DENOMINATOR = 100;

23:     uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;

28:     uint8 public treasuryFeePercent = MAX_TREASURY_FEE_PERCENT;

35:     event SetTreasuryFeePercent(uint8 _treasuryFeePercent);

Mitigation

Generally use the uint256 type and convert it to smaller type when you want to store a value.

[G-04] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Findings

src/vaults/AutoPxGlp.sol
94:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

106:     function setPlatformFee(uint256 fee) external onlyOwner {

118:     function setCompoundIncentive(uint256 incentive) external onlyOwner {

130:     function setPlatform(address _platform) external onlyOwner {

src/vaults/AutoPxGmx.sol
104:     function setPoolFee(uint24 _poolFee) external onlyOwner {

116:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

128:     function setPlatformFee(uint256 fee) external onlyOwner {

140:     function setCompoundIncentive(uint256 incentive) external onlyOwner {

152:     function setPlatform(address _platform) external onlyOwner {

src/PirexFees.sol
63:     function setFeeRecipient(FeeRecipient f, address recipient)
64:         external
65:         onlyOwner
66:     {

83:     function setTreasuryFeePercent(uint8 _treasuryFeePercent)
84:         external
85:         onlyOwner
86:     {

src/PirexGmx.sol
272:     function configureGmxState() external onlyOwner whenPaused {

300:     function setFee(Fees f, uint256 fee) external onlyOwner {

313:     function setContract(Contracts c, address contractAddress)
314:         external
315:         onlyOwner
316:     {

862:     function setDelegationSpace(
863:         string memory _delegationSpace,
864:         bool shouldClear
865:     ) external onlyOwner {

884:     function setVoteDelegate(address voteDelegate) external onlyOwner {

895:     function clearVoteDelegate() public onlyOwner {

909:     function setPauseState(bool state) external onlyOwner {

921:     function initiateMigration(address newContract)
922:         external
923:         whenPaused
924:         onlyOwner
925:     {

956:     function completeMigration(address oldContract)
957:         external
958:         whenPaused
959:         onlyOwner
960:     {


src/PirexRewards.sol
93:     function setProducer(address _producer) external onlyOwner {

151:     function addRewardToken(ERC20 producerToken, ERC20 rewardToken)
152:         external
153:         onlyOwner
154:     {

179:     function removeRewardToken(ERC20 producerToken, uint256 removalIndex)
180:         external
181:         onlyOwner
182:     {

432:     function setRewardRecipientPrivileged(
433:         address lpContract,
434:         ERC20 producerToken,
435:         ERC20 rewardToken,
436:         address recipient
437:     ) external onlyOwner {

461:     function unsetRewardRecipientPrivileged(
462:         address lpContract,
463:         ERC20 producerToken,
464:         ERC20 rewardToken
465:     ) external onlyOwner {

src/PxERC20.sol
45:     function mint(address to, uint256 amount)
46:         external
47:         virtual
48:         onlyRole(MINTER_ROLE)
49:     {

62:     function burn(address from, uint256 amount)
63:         external
64:         virtual
65:         onlyRole(BURNER_ROLE)
66:     {

src/PxGmx.sol
19:     function burn(address from, uint256 amount)
20:         external
21:         override
22:         onlyRole(BURNER_ROLE)
23:     {}

Mitigation

Add payable keyword to a function declaration

[G-05] Unchecked arithmetic

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected. if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

Findings

src/PirexRewards.sol
163:         for (uint256 i; i < len; ++i) {

351:         for (uint256 i; i < pLen; ++i) {

396:             for (uint256 i; i < rLen; ++i) {

src/vaults/AutoPxGlp.sol
168:         return assets - penalty;

penalty is 0 or something smaller than assets.

src/vaults/AutoPxGlp.sol
194:  (FEE_DENOMINATOR - withdrawalPenalty);

FEE_DENOMINATOR is always greater than withdrawalPenalty

src/vaults/AutoPxGlp.sol
250:         uint256 newAssets = totalAssets() - preClaimTotalAssets;

totalAssets() is always >= preClaimTotalAssets

src/vaults/AutoPxGlp.sol
264:         pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;

pxGmx.balanceOf(address(this)) is always >= preClaimPxGmxAmount

src/vaults/AutoPxGlp.sol
276:             pxGmx.safeTransfer(owner, totalPxGmxFee - pxGmxIncentive);

279:             _harvest(pxGmxAmountOut - totalPxGmxFee);

src/vaults/AutoPxGmx.sol
190:         return assets - penalty;

216:         (FEE_DENOMINATOR - withdrawalPenalty);

288:         if ((totalAssets() - assetsBeforeClaim) != 0) {

299:         asset.safeTransfer(owner, totalFee - incentive);

src/vaults/PxGmxReward.sol
117:             globalState.rewards = globalRewards - userRewards;

126:                 rewardState = _rewardState - amount;

src/PirexFees.sol
104:         uint256 contributorsDistribution = distribution - treasuryDistribution;

src/PirexGmx.sol
223:         postFeeAmount = assets - feeAmount;

225:         assert(feeAmount + postFeeAmount == assets);

src/PirexRewards.sol
390:             p.globalState.rewards = globalRewards - userRewards;

407:                     p.rewardStates[rewardToken] = rewardState - amount;

Mitigation

Place the arithmetic operations in an unchecked block

	for (uint i; i < length;) {
		...
		unchecked{ ++i; }
	}

[G-06] Obsolete action

There is no need for this assert:

Findings

src/PirexGmx.sol
225:         assert(feeAmount + postFeeAmount == assets);

Since feeAmount + postFeeAmount is always == assets, just waste of gas by doing this.

Mitigation

Remove it.