[WP-M1] `withdraw()` transactions can often fail

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L344-L344

function _withdraw(
    address _to,
    uint256[] memory _ids,
    bool _force
) internal {
    uint256 localTotalShares = totalShares();
    uint256 localTotalPrincipal = totalUnderlyingMinusSponsored();
    uint256 amount;

    for (uint8 i = 0; i < _ids.length; i++) {
        amount += _withdrawDeposit(
            _ids[i],
            localTotalShares,
            localTotalPrincipal,
            _to,
            _force
        );
    }

    underlying.safeTransfer(_to, amount);
}

In the current implementation, when the user tries to withdraw(), the contract will directly transfer the underlying token to a specified address.

However, since the contract will invest a certain portion (usually a rather large portion) of the funds into the strategy (and then to a 3rd party: EthAnchor), the balance of underlying token in the Vault contract can often be quite low.

PoC

Given:

• investPerc = 80%

1. Alice deposited 50k;
2. Bob deposited 30k;
3. Charlie deposited 20k;
4. 80k invested in EthAnchor, 20k left in the contract balance;
5. Alice and Bob try to withdraw() and their txs will fail.

Impact

Many withdraw() transactions will fail due to insufficient underlying token amounts, especially for withdrawals with larger amounts.

Recommendation

Consider adding a new function requestWithdraw(uint amount):

Check if the current underlying token balance is enough for the withdraw, if true, fulfill the withdrawal immediately;

Otherwise:

1. Make sure requstedWithdrawlAmount[msg.sender] + amount <= depositAmount;
2. requstedWithdrawlAmount[msg.sender] += amount;
3. Call initRedeemStable(amount), return the redeemId.
4. User can call finishWithdraw(uint id) with the redeemId to finalize the withdrawal.

This introduces a way for the users to get money back from the underlying contract (EthAnchor) without relying on a centralized role.

[0xBRM]

This is by design. Partial withdrawals should help with this. Do keep in mind that more rebalances decrease the overall profitability of the vault due to curve's fees. I think partial withdrawals are a good compromise but willing to discuss further.

Current thinking is that

For UST vaults (no curve): users can withdraw immediately as per your solution For non-UST vaults (with curve): users cannot withdraw immediately if the amount of funds is greater than the reserve. Allowing them to would introduce another griefing attack vector (more rebalances, decreased profitability due to curve fees). I suggest making the uninvest function of these vaults callable not only by the backend's EOA, but also by our multisig.

[naps62]

Also, although this is a much more comprehensive description, it's a duplicate of #126

[dmvt]

Duplicate of #76