Sandclock contest - onewayfunction's results

Handle

onewayfunction

Vulnerability details

Impact

Attackers can reenter the deposit function in such a way as to give themselves a higher value of claimers.sharesOf than they ought to have, thereby giving them a larger share of overall yield than they deserve.

Proof of Concept

First we'll show where the reentrancy occurs, and then we'll show how this can be exploited to give an attacker an undue amount of "claimers shares".

The Vault.deposit function calls _createDeposit. Critically, this happens before it calls the _transferAndCheckUnderlying function, which will be important later.

The _createDeposit function does two important things before the reentrancy happens:

• It stores the value of claimers.totalShares() in memory as localTotalShares.
• It stores the contract's balance of the underlying in memory as localTotalUnderlying. (We'll ignore the impact of sponsored funds for the purposes of simplicity -- they have no real relevance wrt this vuln).

Then, _createDeposit calls _createClaim using the above values stored in memory.

Next, _createClaim uses these stored values to compute the number of "claimers shares" to give to the user.

This number of "claimers shares" is then minted by calling claimers.mint, which properly increases the total (global) number of "claimers shares" in existence. Everything is fine up to here.

Next, the _createDeposit function calls depositors.mint, which is where control is handed over to the attacker. Specifically, depositors.mint calls _safeMint(_owner, localTokenId), which, calls _checkOnERC721Received, which then makes an unsafe non-static call to the onERC721Received function on whatever to address originally called the Vault.deposit() function. This is where the attacker can take control of the execution flow -- by calling the Vault.deposit function from a malicious contract that has an external onERC721Received function that will hijack the flow of execution at L476 of Vault.sol.

Now the attacker can reenter the Vault contract and call Vault.deposit again. But notice that, for this inner call, the value of claimers.totalShares() has already been increased by the original/"outer" call to deposit, but the Vault's underlying balance has not yet been increased by the original/"outer" call to deposit (because the _transferAndCheckUnderlying hasn't happened yet for the outer call).

The result is that the inner call to deposit will result in an incorrectly large value for _computeShares, which is computed as

(_amount * _totalShares) / _totalUnderlyingMinusSponsored;

because _totalShares will be correct, but _totalUnderlyingMinusSponsored will be too small. So the attacker will receive more claimer shares than they deserve.

Afterwords, every time they call claimYield, they will a receive larger share of rewards than they deserve, at the expense of honest users. (Indeed, even after eventually withdrawing their original capital, they will still have some claimer shares left over, and so may continue earning yield even though they have no more underlying at stake).

Recommended Mitigation Steps

I recommend using the _mint function instead of the _safeMint function on L53 of Depositors.sol, since it does not make any unsafe external calls.