Sandclock contest - palina's results

Handle

palina

Vulnerability details

Impact

The exchange performed in NonUSTStrategy.sol via Curve is executed with "0" as the minimum amount received as the result of the operation, which is likely to be exploited by front-running and may lead to the loss of funds.

Proof of Concept

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

_swapUstToUnderlying(): https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L94

Tools Used

Manual Analysis

Recommended Mitigation Steps

Set slippage protection parameter when exchanging tokens via Curve (e.g., calculate the minimum acceptable amount received using the knwon exchange rate via e.g., get_dy_underlying()).

Handle

palina

Vulnerability details

Impact

In Vault.sol, totalUnderlying() (and, therefore, totalUnderlyingMinusSponsored()) include both funds available in the Vault as well as those invested in the Strategy. The calculation of amounts returned to depositors and sponsors (in _withdraw() and _unsponsor()) also relies on the values produced by totalUnderlying()/minusSponsored() which include invested funds that are not present in the Vault contract. Therefore, even if the following check in _unsponsor() succeeds: require(sponsorToTransfer <= totalUnderlying(), "Vault: not enough funds to unsponsor");

That is further complicated by the fact that the Vault does not implement any ability to pull funds from the Strategy.

## Proof of Concept
Vault.totalUnderlying():
https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L125

Vault.totalUnderlyingMinusSponsored():
https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L292

_unsponsor():
https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L394

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

## Tools Used
Manual Analysis

## Recommended Mitigation Steps
During withdraw()/unsponsor() check if the amount to be withdrawn is <= the amount of underlying *in possession of the Vault*. Consider adding functionality that allows pulling *some* funds out of the strategy, when necessary.

Handle

palina

Vulnerability details

Impact

Both _mint() and _safeMint() functions are used to mint ERC721 NFTs in Claimers and Depositors, respectively. The usage of the _mint() is, however, discouraged by the used ERC721 implementation (see PoC section), in favor of its safe counterpart.

Proof of Concept

Depositors: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/vault/Depositors.sol#L53

Claimers: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/vault/Claimers.sol#L63

ERC721: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/80d8da05644ceef3cd8e81860882571f037f8667/contracts/token/ERC721/ERC721.sol#L271

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider using _safeMint() in Claimers.mint().