Collateralization ratio can be broken by users redeeming deposits for ETH

[zobront]

Summary

A key property of the DYAD system is that the collateralization ratio (300%) is maintained. This means that for every 1 DYAD in circulation, there is 3x as much ETH (priced in USD) in the vault.

This invariant is enforced in the withdraw() function, which stops users from minting more DYAD when such a mint would break the invariant:

function withdraw(uint from, address to, uint amount) external
    isNftOwnerOrHasPermission(from, Permission.WITHDRAW)
    isUnlocked(from)
{
    _subDeposit(from, amount);

    uint collatVault    = address(this).balance * _getEthPrice()/1e8;
    uint newCollatRatio = collatVault.divWadDown(dyad.totalSupply() + amount);
    if (newCollatRatio < MIN_COLLATERIZATION_RATIO) { revert CrTooLow(); } 
    ...
}

However, the same check is not enforced when redeeming ETH out of the contract. Since a key goal is keeping the ratio of circulating DYAD and ETH bounded by this ratio, it is crucial that we enforce this check on both DYAD minting and ETH redeeming.

Proof of Concept

Here is a test showing that we can get the collateralization ratio as low as 1:1 by withdrawing all non-minted deposits:

function test_CollateralizationRatioBrokenOnRedeemDeposit() public {
    // We deposit 5000 in totalDeposit and mint 1000 of them. Ratio is $5000 of ETH / 1000 supply.
    uint id1 = dNft.mint{value: 5 ether}(address(this));
    dNft.withdraw(id1, address(this), 1000e18);
    console.log(_calculateCollateralizationRatio()); // returns 5e18 - success

    // We can now withdraw all the remaining ETH with no check.
    dNft.redeemDeposit(id1, address(1), 4000e18);
    console.log(_calculateCollateralizationRatio()); // returns 1e18 - uh oh
}

function _calculateCollateralizationRatio() internal returns(uint) {
    uint ETH_PRICE = 1000 * 1e8;
    uint MIN_COLLATERIZATION_RATIO = 3e18;
    uint collatVault = address(dNft).balance * ETH_PRICE/1e8;
    uint newCollatRatio = FixedPointMathLib.divWadDown(collatVault, dyad.totalSupply());
    return newCollatRatio;
}

Recommendation

I would recommend moving the collateralization logic into a modifier, as follows:

modifier collateralizationCheck(uint _amountMinted, uint _amountRedeemed) {
    uint collatVault = (address(this).balance - _dyad2eth(_amountRedeemed)) * _getEthPrice() / 1e8;
    if (dyad.totalSupply() > 0) {
        uint newCollatRatio = collatVault.divWadDown(dyad.totalSupply() + _amountMinted);
        if (newCollatRatio < MIN_COLLATERIZATION_RATIO) { revert CrTooLow(); }
    }
    _;
}

This modifier could then be implemented by both the functions listed below:

function withdraw(uint from, address to, uint amount) external
    isNftOwnerOrHasPermission(from, Permission.WITHDRAW)
    isUnlocked(from)
    collateralizationCheck(amount, 0) 
{ ... }

and

function redeemDeposit(uint from, address to, uint amount) external
    isNftOwnerOrHasPermission(from, Permission.REDEEM)
    isUnlocked(from)
    collateralizationCheck(0, amount)
    returns (uint) { ... }

You'll note a few small additional changes:

• We need to check whether dyad.totalSupply() == 0 before performing this logic, because the collateralization ratio is infinite before any DYAD has been minted, and we will revert when dividing by zero in our check. This is included in the modifier above.
• Your existing testCannot_WithdrawCrTooLow test is broken by this change, but it appears this test is incorrect. It is expecting a revert when a dNFT is minted and all is withdrawn, but this situation should be fine, as it does not break any collateralization ratio is there is no DYAD yet in existence.

[shafu0x]

proposed fixed https://github.com/DyadStablecoin/contracts-v3/pull/20/files

[zobront]

Very nice work. This is awesome. I left a few comments in the PR.

[shafu0x]

@zobront Sorry I can not see the comments?

[zobront]

Fixed by #20.