Is this function vulnerable to a reentrancy attack?

Smart Contracts
1 
function Redeem(uint256 _uid) public {
    require(msg.sender == ownerOf(_uid));
    uint256 total = _tokenIdCounter.current() - _BurntCounter.current();
    uint256 share = (address(this).balance) / total;
    payable(msg.sender).transfer(share);
    burn(_uid);
    _BurntCounter.increment();
}

I am worried about reentrancy, but if the transaction was interrupted before the burn() and _BurntCounter.increment(), then the transfer() would be reverted also right?

It seems to me like reentrancy is really only an issue when you allow calls to other functions, but I want to make sure.

2

Transfer function forwards only 2300 gas only. Is that enough for re-entrancy ?

3

That's a good question, I don't actually know.

4

It is totally vunerable to reentracy attacks. A smart contract holding _uid can call this function and have the receive function designed to call back this function again such that he can get paid by share amount multiple times without getting _uid burned.

Therefore, it's better to be safe than sorry. If I were you, I would put the payable(msg.sender).transfer(share); function to the bottom of the function and add nonReentrant modifier.

5

A contract can't receive wei using "transfer"

1 Like
6

Just tried v0.8.9 and it can.

1 Like
7

Thanks friends! I didn't know about the reentrancy modifier.

8

Its not a default function, its one of openzeppelins contracts you can add.

As for the transfer and send function they should not be used anymore, instead the call function should be used. As mentioned above those will use a fixed gas amount which with the changes in gas cost might not be enough anymore.

9

Can you please provide a PoC?

11

CryptoWorld is right, you have to avoid the transfer( ) method. Try call ( ) instead. Somethinh like:

(bool success, ) = payable(msg.sender).call{value: share}("");
 require(success, "failed");
12

If you use .call then you allow a reentrancy