code-423n4 / 2022-09-vtvl-findings

0 stars 0 forks source link

Missing ReEntrancy Guard to Withdraw function #478

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L364-L392

Vulnerability details

Impact

Missing ReEntrancy Guard to Withdraw function

Proof of Concept

There is no re-entry risk on true ERC-20 tokens that work according to the spec (i.e. audited, etc.).

However you can write a malicious ERC-20 with custom transferFrom() or approve() that have re-entrancy hooks to attack a target.

Furthermore ERC-777 is backwards compatible token standard with ERC-20 standard. ERC-777 has better usability, but it has transfer hooks that can cause re-entrancy.

Project developers have mentioned Reentrancy with the information below. However, to prevent Reentrancy, only the Checks-effects-interaction pattern is not enough.

VTVLVesting.sol#L385
         // After the "books" are set, transfer the tokens
         // Reentrancy note - internal vars have been changed by now
         // Also following Checks-effects-interactions pattern

openzeppelin's view on the reentrancy problem

ERC20 generally doesn't result in reentrancy, however ERC777 tokens can and they can maskerade as ERC20. So if a contract interacts with unknown ERC20 tokens it is better to be safe and consider that transfers can create reentrancy problems.

Although reentrancy attack is considered quite old over the past two years, there have been cases such as:

Type of Reentrancy

Details 1 - Single Function Reentrancy 2 - Cross-Function Reentrancy 3 - Cross-Contract Reentrancy

Tools Used

Manuel code review

Recommended Mitigation Steps

Use Openzeppelin or Solmate Re-Entrancy pattern

Here is a example of a re-entracy guard


// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

contract ReEntrancyGuard {
    bool internal locked;

    modifier noReentrant() {
        require(!locked, "No re-entrancy");
        locked = true;
        _;
        locked = false;
    }
}
0xean commented 2 years ago

dupe of #6