Sandclock contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

The _withdrawDeposit function has the following code which runs before the underlying tokens are transferred to the to address:

if (_isIntegration(_claim.beneficiary)) {
	bytes4 ret =  IIntegration(_claim.beneficiary).onDepositMinted(
		tokenId,
		newShares,
		_claim.data
	);
	require(
		ret ==  IIntegration(_claim.beneficiary).onDepositMinted.selector
	);
}

Also, notice that the return value of _withdrawDeposit is computed as ((_totalUnderlyingMinusSponsored * _shares) / _totalShares). Now, none of the functions have reentrancy guards, so using the two facts above, a user can call withdraw, reenter half-way through this call with a lower _totalShares value and the same _totalUnderlyingMinusSponsored value, which means an overall higher number of underlying tokens would be returned. This is a theft of other people's deserved underlying tokens, so this is a high severity issue.

Proof of Concept

Suppose that Bob sets up a deposit where the beneficiary is a contract that belongs to him. Bob implements the beneficiary to match the IIntegration interface, except he puts custom logic inside of the onDepositBurned function. This logic will simply reenter the withdraw function, and notice that when Bob calls withdraw on this deposit, this reentry happens before the original call transfers the underlying tokens to Bob. So, the inner withdraw will be operating on the same _totalUnderlyingMinusSponsored value, but with a reduced _totalShares value. This results in a larger value being returned from _withdrawDeposit, and thus Bob is transferred a larger amount of tokens than he should be. With large enough deposits/more reentry nesting, Bob would be able to drain the contract of most of its funds.

Tools Used

Inspection.

Recommended Mitigation Steps

Add reentrancy guards to the withdraw function so that a user can't use this attack vector to steal underlying tokens.

Handle

harleythedog

Vulnerability details

Impact

The function _swapUstToUnderlying exists to swap Ust to underlying tokens. The last argument to exchange_underlying is min_dy, which specifies the minimum number of underlying to be returned from the swap. Currently, this value is set to 0, so the function is subject to a sandwich attack. Specifically, an attacker can watch for transactions that call _swapUstToUnderlying, and then (using flashbots) execute a sandwich attack to manipulate the price of the underlying/Ust pair before and after the swap. This can steal a very good chunk of the swap, so this represents a loss to the protocol.

Proof of Concept

See _swapUstToUnderlying here: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L90

Tools Used

Inspection.

Recommended Mitigation Steps

Should allow the admin to optionally specify the min_dy amount in calls to _swapUstToUnderlying, so that sandwich attacks revert if they are attempted by attackers.

Handle

harleythedog

Vulnerability details

Impact

In Vault.sol, the deposit function is the external function that allows transferring underlying tokens to mint position NFTs. The deposit function first calls _createDeposit (which creates the position/determines how many shares to allocate), and then calls _transferAndCheckUnderlying (transfers the underlying tokens from msg.sender).

The issue is that the _createDeposit function determines how many shares to allocate based on the balance of the underlying tokens, which doesn't get incremented until _transferAndCheckUnderlying. So, if there is a reentrancy opportunity in _createDeposit, then a user can use this to their advantage to get more shares than they really should. See below for a proof of concept.

The result of this is that an attacker can steal funds that have been deposited by other users, since the attacker is able to receive more shares than they should based on the amount they transfer in. Since this is a theft of other user's funds, this is clearly a high severity issue.

Proof of Concept

Suppose that Alice is a normal user, and Bob is an attacker.

• Alice interacts normally with the contract, and for simplicity lets assume that she is the only user. Suppose she owns 10 claim shares, and the entire balance of Vault.sol is 100 tokens.
• Now, suppose that Bob calls deposit with a malicious contract that implements onERC721Received to call vault.deposit. Now, suppose that Bob calls deposit with 100 tokens. Bob will be allocated _amount * _totalShares / _totalUnderlyingMinusSponsored = 100 * 10 / 100 = 10 claimer shares. When depositors.mint is executed on line 476, Bob is able to reenter deposit (since _safeMint calls onERC721Received on the owner. See https://twitter.com/not__stoops/status/1462638499316699137 for another example of a project that had this reentrancy vulnerability), and Bob chooses to deposit another 100 tokens. At this point, his initial 100 tokens have not been transferred (so _totalUnderlyingMinusSponsored is still 100), but totalShares.totalShares() will have been incremented by the 10 he was minted when claimers.mint was executed on line 470. This means that Bob will receive _amount * _totalShares / _totalUnderlyingMinusSponsored = 100 * 20 / 100 = 20 claimer shares for this inner deposit.
• After the inner and outer transactions complete, Bob will have transferred 200 tokens and now owns 30 out of the 40 claimer shares of the 300 underlying tokens. This means that Bob owns 3/4 of the shares, but he only transferred in 2/3 of the underlying tokens. Bob clearly has been allocated more shares than he should have at this point, and is thus able to withdraw tokens that rightfully belong to Alice.

Tools Used

Inspection and tested the POC with hardhat.

Recommended Mitigation Steps

Add reentrancy guards to the deposit function so that a user can't use this attack vector to get more shares.

Handle

harleythedog

Vulnerability details

Impact

When a user calls withdraw, the amount of underlying assets that they are owed is dependent on their number of shares, and the number of underlying assets in the vault + strategy. If x is the number of underlying tokens intended to be sent to the user in a call to withdraw, then it is certainly possible that the balance of the vault is less than x (as most of the tokens are locked up in the strategy).

In this case, whenever the user calls withdraw, the call will revert, and there will need to be manual intervention from an admin to transfer the appropriate amount of tokens from the strategy back to the vault using withdrawToVault.

This is not very ideal, and this is actually an issue, since users will not be able to withdraw from their positions when they want - they will need to rely on manual intervention from the admins. Once the lockedUntil timestamp passes, a user should be able to withdraw from their position whenever they desire.

Proof of Concept

See the code for _withdraw here: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L325

Notice that there are no calls to the strategy here, so if there are not the correct number of underlying tokens in the strategy for the user, the call to withdraw will revert.

Tools Used

Inspection.

Recommended Mitigation Steps

The vault should determine if extra tokens are needed to be withdrawn from the strategy, and if necessary the vault should call withdrawToVault to get tokens from the stategy. This way, users will always be able to withdraw from their positions once their lockedUntil timestamp has expired.

Handle

harleythedog

Vulnerability details

Impact

There are several ERC20 tokens that take a small fee on transfers/transferFroms (known as "fee-on-transfer" tokens). Most notably, USDT is an ERC20 token that has togglable transfer fees, but for now the fee is set to 0 (see the contract here: https://etherscan.io/address/0xdAC17F958D2ee523a2206206994597C13D831ec7#code).

In the function _transferAndCheckUnderlying in Vault.sol, we have the following code:

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"
        );
    }

With fee-on-transfer underlying tokens, this will always revert, as calling safeTransferFrom(_from, address(this), _amount) will NOT increase the balance of the contract by _amount (since you have to subtract the fee). As a result, USDT and other fee-on-transfer tokens can't be used as underlying tokens, as all calls to deposit will revert. I believe that USDT is a desired underlying token (based on the slidedeck on the sandclock website) so this should be fixed.

Proof of Concept

See the code for _transferAndCheckUnderlying here: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L580.

Tools Used

Inspection.

Recommended Mitigation Steps

I recommend removing the require statement altogether. It doesn't seem to actually achieve anything, as with safeTransfer you can already assume that the transaction will revert if something goes wrong.

Handle

harleythedog

Vulnerability details

Impact

In the function setStrategy within Vault.sol, there is a requirement that strategy.investedAssets() == 0 (so that no funds are left stuck in the strategy). A malicious user could strategically transfer 1 wei of underlying to the strategy (or any other relevant token) in order to intentionally make setStrategy revert.

This is even easier since setStrategy is only callable by the admins, so if the admin is a multi-sig timelock (which is ideal), then it would be very easy to time this grief. If the admin is indeed a timelock, it would be even worse, as it could take multiple days for another call to setStrategy to be executed, so it would be a very annoying grief at almost no cost to the attacker.

Proof of Concept

See the code for setStrategy here: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L103.

Tools Used

Inspection.

Recommended Mitigation Steps

I recommend calling withdrawAllToVault on the strategy at the start of setStrategy. This would make it so the strategy transfers all its funds in the same transaction as setStrategy is called, so no griefing attack is possible.