Sandclock contest - WatchPug's results

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) / _totalUnderlyingMinusSponsored and _totalUnderlyingMinusSponsored is 1, 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.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L74-L85

function _swapUnderlyingToUst() internal {
    uint256 underlyingBalance = _getUnderlyingBalance();
    if (underlyingBalance > 0) {
        // slither-disable-next-line unused-return
        curvePool.exchange_underlying(
            underlyingI,
            ustI,
            underlyingBalance,
            0
        );
    }
}

The current implementation of doHardWork() and finishRedeemStable() -> _swapUnderlyingToUst() and _swapUstToUnderlying() provides no parameter for slippage control, making it vulnerable to front-run attacks.

Recommendation

Consider adding a amountOutMin parameter.

Handle

WatchPug

Vulnerability details

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

uint256 tokenId = depositors.mint(
    _msgSender(),
    amount,
    claimerId,
    _lockedUntil
);

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/node_modules/@openzeppelin/contracts/token/ERC721/ERC721.sol#L256-L266

function _safeMint(
    address to,
    uint256 tokenId,
    bytes memory _data
) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, _data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}

An early malicious user can send 1 wei of the vault token to the contract (as its wallet balance) then call deposit() as the first depositor of the Vault.

Beacuse deposit() -> depositors.mint() will mint a NFT to _msgSender() which can be an contract, this opens the oppotunity of reentrancy attck: the attacker can call deposit() again in onERC721Received().

As a result, the contract can malfunction or even freeze users' funds if the attack is planned sophisticatedly.

In the PoC bellow, we demoed an attack that essentially planted a bomb to the contract, which will be triggered once 1M are deposited and freeze users' funds for overflow due to the extremely large _totalShares.

PoC

1. Attacker send 1 wei of underlying token to the contract.
2. Attacker call deposit() with 340282366920938496 wei:

• newShares = 340282366920938496 * 10**18 (SHARES_MULTIPLIER)
• totalShares = newShares;

In onERC721Received(), Attacker call deposit() with 340282366920938496 wei again:

• newShares = (340282366920938496 * 340282366920938496 * 10**18) / 1
• totalShares = 115792089237316217907133152335680512000000000000000000;

💣 The bomb is now planted.

3. Bob call deposit() with 100,000 * 1e18 of underlying token:

• localTotalPrincipal = 340282366920938496 *2 + 1
• newShares = (100,000 * 1e18 * 115792089237316217907133152335680512000000000000000000) / localTotalPrincipal
• totalShares = 17014234138136162141217907133152335643777801536803518942350;

4. Charlie call deposit() with 1,000,000 * 1e18 of underlying token:

• localTotalPrincipal = 340282366920938496 *2 + 1 + 100,000 * 1e18
• newShares = (1,000,000 * 1e18 * 17014234138136162141217907133152335643777801536803518942350) / localTotalPrincipal
• totalShares = 32481710148606587477395626066288346473515628339947249220056;

💥 The bomb is now triggered!

5. Charlie tries to call withdraw():

• localTotalShares = 32481710148606587477395626066288346473515628339947249220056
• localTotalPrincipal = 1100000680564733841876993;
• uint256 max = 115792089237316195423570985008687907853269984665640564039457584007913129639935;
• _amount * _totalShares = 187155417598605410391217907133152335276435816904838708365850000000000000000000000000;

The transaction will revert at _computeShares() due to overflow on _amount * _totalShares.

Recomandation

Consider using ReentrancyGuard from OpenZeppelin, add nonReentrant() modifiers to deposit() and other important entry functions.

Handle

WatchPug

Vulnerability details

Even though it's unlikely in practice, but in theory, the underlying contract (EthAnchor) may suffer investment losses and causing decreasing of the PPS of AUST token. (There are codes that considered this situation in the codebase. eg. handling of depositShares > claimerShares).

However, when this happens, the late users will suffer more losses than expected than the users that withdraw earlier. The last few users may lose all their funds while the first users can get back 100% of their deposits.

PoC

// ### for deposits: d1, d2, d3, the beneficiary are: c1, c2, c2
    depositAmount          claimerShares
d1: + 100e18           c1: + 100e36
d2: + 100e18           c2: + 100e36
d3: + 100e18           c2: + 100e36

depositAmount of d1, d2, d3 = 100e18
c1 claimerShares: 100e36
c2 claimerShares: 200e36
total shares: 300e36


// ### when the PPS of AUST drop by 50% 
totalUnderlyingMinusSponsored: 300e18 -> 150e18

// ### d2 withdraw
c2 claimerShares: 200e36
d2 depositAmount: 100e18
d2 depositShares: 300e36 * 100e18 / 150e18 = 200e36

Shares to reduce: 200e36
c2 claimerShares: 200e36 -> 0
c2 totalPrincipal: 200e18 -> 100e18
totalShares: 300e36 -> 100e36

underlying.safeTransfer(d2, 100e18)
totalUnderlyingMinusSponsored: 150e18 -> 50e18

Root Cause

When the strategy is losing money, share / underlying increases, therefore the computed depositShares: depositAmount * share / underlying will increase unexpectedly.

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

While totalShares remain unchanged, but the computed depositShares is increasing, causing distortion of depositShares / totalShares, eg, ∑ depositShares > totalShares.

Recommendation

In order to properly handle the investment loss of the strategy, consider adding a new storage variable called totalLoss to maintain a stable value of share / adjustedUnderlying.

adjustedUnderlying = underlying + totalLoss

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L66-L69

NonUSTStrategy will swap the deposited non-UST assets into UST before depositing to EthAnchor. However, the swap fee is not attributed to the depositor correctly like many other yield farming vaults involving swaps (ZapIn).

An attacker can exploit it for the swap fees paid by other users by taking a majority share of the liquidity pool.

Root Cause

The swap fee of depositing is not paid by the depositor but evenly distributed among all users.

PoC

Given:

• A NonUST vault and strategy is created for FRAX;
• The liquidity in FRAX-UST curve pool is relatively small (<$1M).

The attacker can do the following:

1. Add $1M worth of liquidity to the FRAX-UST curve pool, get >50% share of the pool;
2. Deposit 1M FRAX to the vault, get a depositAmount of 1M;
3. The strategy will swap 1M FRAX to UST via the curve pool, paying a certain amount of swap fee;
4. Withdraw all the funds from the vault.
5. Remove the liquidity added in step 1, profit from the swap fee. (A majority portion of the swap fee paid in step 3 can be retrieved by the attacker as the attacker is the majority liquidity provider.)

If the vault happens to have enough balance (from a recent depositor), the attacker can now receive 1M of FRAX.

A more associated attacker may combine this with issue [WP-M4] and initiate a sandwich attack in step 3 to get even higher profits.

As a result, all other users will suffer fund loss as the swap fee is essentially covered by other users.

Recommendation

Consider changing the way new shares are issued:

1. Swap from Vault asset (eg. FRAX) to UST in deposit();
2. Using the UST amount out / total underlying UST for the amount of new shares issued to the depositor.

In essence, the depositor should be paying for the swap fee and slippage.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Handle

WatchPug

Vulnerability details

The current implementation requires trusted key holders (isTrusted[msg.sender]) to send transactions (initRedeemStable()) to initialize withdrawals from EthAnchor before the users can withdraw funds from the contract.

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/BaseStrategy.sol#L214-L223

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/BaseStrategy.sol#L163-L170

This introduces a high centralization risk, which can cause funds to be frozen in the contract if the key holders lose access to their keys.

PoC

Given:

• investPerc = 80%
• 1,000 users deposited 1M UST in total ($1000 each user in avg), 800k invested into AUST (EthAnchor)

If the key holders lose access to their keys ("hit by a bus"). The 800k will be frozen in EthAnchor as no one can initRedeemStable().

Recommendation

See the recommendation on issue [WP-M1].

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

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.

Handle

WatchPug

Vulnerability details

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

function totalUnderlyingMinusSponsored() public view returns (uint256) {
    // TODO no invested amount yet
    return totalUnderlying() - totalSponsored;
}

As a function that many other functions depended on, totalUnderlyingMinusSponsored() can revert on underflow when sponsorAmount > totalUnderlying() which is possible and has been considered elsewhere in this contract:

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

if (_force && sponsorAmount > totalUnderlying()) {
    sponsorToTransfer = totalUnderlying();
}

PoC

• Underlying token = USDT
• Swap Fee = 0.04%

1. Sponsor call sponsor() and send 10,000 USDT

• totalSponsored = 10,000

2. NonUSTStrategy.sol#doHardWork() swapped USDT for UST

• pendingDeposits = 9,996
• totalUnderlying() = 9,996

3. Alice tries to call deposit(), the tx will revet due to underflow in totalUnderlyingMinusSponsored().

Recommendation

Change to:

function totalUnderlyingMinusSponsored() public view returns (uint256) {
    uint256 _totalUnderlying = totalUnderlying();
    if (totalSponsored > _totalUnderlying) {
        return 0;
    }
    return _totalUnderlying - totalSponsored;
}

Handle

WatchPug

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

Vault.sol#_transferAndCheckUnderlying() requires that the received amount is the same as the transfer amount, otherwise, it will revert at L587.

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

function _transferAndCheckUnderlying(address _from, uint256 _amount)
    internal
{
    uint256 balanceBefore = totalUnderlying();
    underlying.safeTransferFrom(_from, address(this), _amount);
    uint256 balanceAfter = totalUnderlying();

    require(
        balanceAfter == balanceBefore + _amount,
        "Vault: amount received does not match params"
    );
}