sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

cheatcode - Reentrancy Vulnerability in FlatcoinVault Contract #281

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 8 months ago

cheatcode

medium

Reentrancy Vulnerability in FlatcoinVault Contract

Summary

The sendCollateral function in FlatcoinVault is vulnerable to reentrancy attacks because it calls external contracts without checks. This could allow attackers to drain funds or manipulate state.

Impact

The impact of this vulnerability is high. A successful exploit could:

Exploit Scenario

An example exploit scenario:

  1. Attacker deploys a malicious contract
  2. FlatcoinVault calls sendCollateral to pay out collateral to the malicious contract
  3. The malicious contract has a fallback function that calls back into sendCollateral
  4. The reentrant call succeeds since state is not yet locked
  5. More collateral is drained via the second call
  6. This can repeat multiple times until the contract balance is drained

Vulnerable Code

The vulnerability arises from sendCollateral calling external contracts without checks:

function sendCollateral(address to, uint256 amount) external onlyAuthorizedModule {
  collateral.safeTransfer(to, amount); 
}

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L138

Recommended Mitigation

To mitigate this, sendCollateral should apply a reentrancy guard or follow the Checks-Effects-Interactions pattern, e.g.:

bool locked;

modifier noReentrancy() {
  require(!locked, "No reentrancy");

  locked = true;
  _;
  locked = false;
}

function sendCollateral() external noReentrancy {
  // ...
} 

This would prevent reentrant calls, fixing the vulnerability.

sherlock-admin commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

nevillehuang commented 7 months ago

Invalid, insufficient proof that reentrancy can occur. You cannot reenter a standard ERC20 transfer. Additionally, agree with sponsors comments:

sendCollateral is guarded to be called only my modules, and all of the calling functions are guarded with non reentrant modifier