Y2k Finance contest - wagmi's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L378

Vulnerability details

Impact

In each Vault epoch, idFinalTVL[] and idClaimTVL[] values is only set when someone call triggerDepeg() or triggerEndEpoch() from Controller. In case of depeg, we will swap assets of hedge and risk Vault.

But if one Vault has noone deposited, idFinalTVL[] will be zero and it makes beforeWithdraw() always reverted because of divide by zero error. The result is funds cannot withdraw() their assets.

Also if only one Vault has be deposited, users will receive nothing. (Depeg: they lost deposited funds, End Epoch: nothing since other Vault is empty)

Proof of Concept

Consider the scenario

1. We have 2 Vault hedge and risk vHedge and vRisk. Alice deposited 10 WETH to vHedge and vRisk has nothing.
2. In case of depeg, we will swap assets of 2 Vault. Which means it transfer 10 WETH from vHedge to vRisk. And since no one deposited to vRisk, these WETH is locked forever.
3. In case of end epoch, vHedge Vault will receive nothing too since only vHedge vault has assets.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider add a mechanism in case noone deposit in 1 Vault after epoch start, consider that epoch is cancelled and users can withdraw their funds.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/test/FuzzTest.t.sol#L123

Vulnerability details

Detail

In Tests section, sponsors said they would like to assess if testFuzzDeposit() and testFuzzWithdraw() are failed due to a logic fault in the contracts or in testing.

For both tests, we only assume that

vm.assume(ethValue >= 1);

So in when fuzzing, if ethValue is too large, for example ethValue = 2**255, it may make some calculation overflow (even it’s just a simple sumation because ethValue is already large).

Proof of Concept

In case ethValue = 2**256 - 1, some calculations will be overflow. For example

When deposit(), it will call to ​​previewDeposit() => convertToShares() with assets = 2**256 - 1.

function convertToShares(uint256 id, uint256 assets)
    public
    view
    virtual
    returns (uint256)
{
    uint256 supply = totalSupply(id); // Saves an extra SLOAD if totalSupply is non-zero.

    return
        supply == 0 ? assets : assets.mulDivDown(supply, totalAssets(id));
}

It will be overflowed in mulDivDown and test failed

Recommendation

Should add a condition on ethValue to be reasonable like

vm.assume(ethValue < (2 << 127));