Sandclock contest - Fitraldys's results

Handle

Fitraldys

Vulnerability details

Impact

When interacting with the vault contract, the user can call the deposit function, however this can be used to reenter this function because the deposit function will eventually call depositors.mint and in the mint function inside depositors will call _safeMint which belong to erc721, this _safeMint function will call _checkOnERC721Received which will make an external call to the user malicous contract, and the malicious user can reenter the deposit function, with totalShare value bigger than the previous call, because https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L470 claimers.mint is called first to increase the share, then the depositors.mint that make the external call, and with this vuln, each reenter will increase the totalShare but the underlying balance is still the same, and this will broke the _computeShares, because the transfer is happening after the _createdeposit is finish.

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L160

Recommended Mitigation

add noreentrant modifier