sherlock-audit / 2023-01-derby-judging

4 stars 1 forks source link

tsvetanovv - Reentrancy attacks in `mintNewBasket()` function #391

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

tsvetanovv

high

Reentrancy attacks in mintNewBasket() function

Summary

In [Game.sol]() we have mintNewBasket function. This function has reentrancy a vulnerability.

function mintNewBasket(uint256 _vaultNumber) external returns (uint256) {
    // mint Basket with nrOfUnAllocatedTokens equal to _lockedTokenAmount
    baskets[latestBasketId].vaultNumber = _vaultNumber;
    baskets[latestBasketId].lastRebalancingPeriod = vaults[_vaultNumber].rebalancingPeriod + 1;
    _safeMint(msg.sender, latestBasketId); 
    latestBasketId++;

    emit BasketId(msg.sender, latestBasketId - 1);
    return latestBasketId - 1;
  }

Vulnerability Detail

The root of the problem are in the mintNewBasket who use _safeMint. This mint call the function onERC721Received of the token recipient, generating a possible reentrancy attack. When writing or interacting with callback functions in solidity, it's important to ensure that they can't be used to perform unexpected effects. _safeMint is a callback function, the recipient contract may define any arbitrary logic to be executed, including reenterring the initial mint function, thereby bypassing limits defined in the contract code.

Impact

It is possible Reentrancy in mintNewBasket().

Code Snippet

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Game.sol#L255-L264

Tool used

Manual Review

Recommendation

Add nonReentrant modifier to `mintNewBasket()`` function.

Duplicate of #1