Sandclock contest - pedroais's results

Handle

pedroais

Vulnerability details

Impact

Dangerous external calls in the middle of various state changes could cause reentrancy issues since there is no reentrancy guard in any functions.

Proof of Concept

When users call the deposit or sponsor functions a deposit NFT is minted. The _safeMint() function that's used makes an unsafe external call to the receiver contract that could reenter any function of the protocol.

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

The safe mint function calls _checkOnERC721Received which will make an external call to the contract receiving the NFT.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3458c1e8541ce0a0cd935828c9db8f9cbca988a0/contracts/token/ERC721/ERC721.sol#L263

An example of safeMint being used for reentrancy can be found in this blog post by samczsun : https://www.paradigm.xyz/2021/08/the-dangers-of-surprising-code/

Recommended Mitigation Steps

Add reentrancy guard to all public statechanching functions.

Handle

pedroais

Vulnerability details

Impact

Users may have to wait more time than predicted to withdraw funds.

Proof of Concept

The vault's funds are distributed between the vault and the investment strategy but withdrawals come only from funds that are currently inside the vault. This means that multiple depositors (or one big depositor) may not be able to withdraw their funds at the same time.

-Alice and bob deposit 100 tokens each so the total balance is 200 -50% of the funds are invested -There are 100 tokens in the vault and 100 in the strategy -If bob withdraws his funds then Alice can't do it anymore and has to wait for her tokens to be brought back to the vault by the admins -This means if you're not one of the firsts to withdraw you may have to wait an arbitrary amount of time (dependant on admins) to get your tokens back