search

code-423n4 / 2022-01-sherlock-findings

0 stars 0 forks source link

Re-entrancy protection #274

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pauliax

Vulnerability details

Impact

Some functions could be strengthened by adding a nonReentrant modifier:

e.g. _restake first invokes _sendSherRewardsToOwner, and _sendSherRewardsToOwner does not follow Checks-Effects-Interactions pattern: first, it sends the rewards and only then deletes the entry.

    // Transfers the SHER tokens associated with this NFT ID to the address of the NFT owner
    sher.safeTransfer(_nftOwner, sherReward);
    // Deletes the SHER reward mapping for this NFT ID
    delete sherRewards_[_id];

While I think the current implementation of the SHER token is safe from re-entrancy (does not contain callback hooks), it is always a good practice to follow the aforementioned Checks-Effects-Interactions pattern to secure the codebase in case the token or other changes are introduced in the future.

Another example is the function execute in SherBuy, in the middle of execution it calls sherlockPosition.initialStake, and initialStake performs _safeMint(_receiver, _id); This _safeMint performs a callback call onERC721Received on the receiver, thus the sender can re-enter this function again.

Recommended Mitigation Steps

While I haven't found any significant way to exploit this, I still suggest revisiting all the functions and applying re-entrancy protection where external calls are performed and the Checks-Effects-Interactions pattern is not followed.

Evert0x commented 2 years ago

60

jack-the-pug commented 2 years ago

Should be a non.