Redacted Cartel contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/9e9bb60f117334da7c5d851646a168ca271575fc/src/vaults/AutoPxGmx.sol#L199

Vulnerability details

Proof of Concept

In both contracts, the previewWithdraw function has the same body

// 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);

The problem is that the ERC4626 standard specifically requires previewWithdraw to round up, while this code does not do so - actually convertToShares rounds down.

previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the vault won't be shortchanged.

Impact

Since contracts are not compliant with a standard this can result in some integration issues and in this particular case it can also result in a wrong amount returned from previewWithdraw which won’t actually work with withdraw, so it can cause lots of reverted transactions. Medium severity is appropriate here.

Recommendation

Make the previewWithdraw method round up, the same way it does in PirexERC4626.sol

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/9e9bb60f117334da7c5d851646a168ca271575fc/src/vaults/PirexERC4626.sol#L80

Vulnerability details

Proof of Concept

The normal ERC4626 implementation (which is not changed in the repository) has a vulnerability which can result in the first depositor stealing every subsequent depositor’s funds.

It works like this:

1. Vault is just deployed and Bob deposits just 1 wei of underlying, so he now holds 1 share
2. Alice is about to deposit 1000 * 1e18 worth of underlying
3. Bob sees this in the mempool and front runs her transaction with a direct ERC20::transfer to the vault for 1000 * 1e18 tokens
4. Now her deposit will result in 0 shares, because the amount she deposited was less than the balance of the contract
5. Now Bob backruns her deposit by redeeming the whole underlying balance for his 1 share (the total supply) resulting in him stealing all of Alice’s deposited tokens

Impact

This can result in a 100% loss of deposited funds for users of the protocol, so it should be of High severity.

Recommendation

Revert when the shares minted are zero.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/9e9bb60f117334da7c5d851646a168ca271575fc/src/vaults/AutoPxGmx.sol#L242

Vulnerability details

Proof of Concept

The compound method has the amountOutMinimum parameter, which basically serves as the slippage tolerance parameter. The problem is that everywhere in the code where compound is called, the value of amountOutMinimum is just 1 wei, which basically means all swaps executed in the compound method can be sandwiched and the user can lose a huge amount of value due to slippage.

Impact

This vulnerability can easily be exploited by a MEV bot that is scouting the public mempool. Setting severity to Medium because that’s what MEV attacks usually get rated as.

Recommendation

Add a setter to dynamically configure slippage tolerance amounts on-chain, or always make sure all transactions go through a private/dark mempool

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/9e9bb60f117334da7c5d851646a168ca271575fc/src/PirexRewards.sol#L373

Vulnerability details

Proof of Concept

The claim method in PirexRewards iterates over the rewardTokens array for a producerToken. Now this array is completely managed by the contract’s owner who can call addRewardToken which pushes a new value in that array, as many times as he decides with whatever value he decides.

Let’s look at the following scenario:

1. Owner has turned malicious or is compromised
2. Owner calls addRewardToken a huge amount of times, which results in the rewardTokens array being huge
3. Now when a user calls claim the code will take a crazy amount of gas, because it has to iterate over the whole rewardTokens array. Since the amount of gas will possibly be more than the block gas limit, this results in a DoS and the user won’t be able to claim.

Or this scenario:

1. Owner has turned malicious or is compromised
2. Owner calls addRewardToken with an address that is not really a token
3. Now when a user calls claim the code will try to do a call to producer.claimUserReward giving the address of the rewardToken, but since it is not really a token (or maybe it is just a random address with no bytecode) the call will revert. Actually it will always revert, which is a DoS state.

Impact

This is a centralisation vulnerability allowing the owner to stop the user rewards anytime. Since it requires a malicious/compromised owner it is Medium severity

Recommendation

Add a way for the user to claim rewards only from a token he chooses, not to have to go through all reward tokens on each claim.