[WP-H12] `forceUnsponsor()` may open a window for attackers to manipulate the `_totalShares` and freeze users' funds at a certain deposit amount

[code423n4]

Handle

WatchPug

Vulnerability details

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

if (_force && sponsorAmount > totalUnderlying()) {
    sponsorToTransfer = totalUnderlying();
} else if (!_force) {
    require(
        sponsorToTransfer <= totalUnderlying(),
        "Vault: not enough funds to unsponsor"
    );
}

totalSponsored -= sponsorAmount;

underlying.safeTransfer(_to, sponsorToTransfer);

When sponsorAmount > totalUnderlying(), the contract will transfer totalUnderlying() to sponsorToTransfer, even if there are other depositors and totalShares > 0.

After that, and before others despoiting into the Vault, the Attacker can send 1 wei underlying token, then cal deposit() with 0.1 1e18 , since `newShares = (_amount _totalShares) / _totalUnderlyingMinusSponsoredand_totalUnderlyingMinusSponsoredis1, with a tiny amount of underlying token,newShares` will become extremly large.

As we stated in issue [WP-H10], when the value of totalShares is manipulated precisely, the attacker can plant a bomb, and the contract will not work when the deposit/withdraw amount reaches a certain value, freezing the user's funds.

However, this issue is not caused by lack of reentrancy protection, therefore it cant be solved by the same solution in [WP-H10].

Recomandation

Consider adding a minimum balance reserve (eg. 1e18 Wei) that cannot be withdrawn by anyone in any case. It can be transferred in alongside with the deployment by the deployer.

This should make it safe or at least make it extremely hard or expensive for the attacker to initiate such an attack.

[naps62]

@gabrielpoca @ryuheimat is this new?

[r2moon]

it's new

[gabrielpoca]

yap, it's interesting. The sponsor really is an issue

[naps62]

forceUnsponsor has since been removed