JuggerNaut63 - Reentrancy Exploit in Yield Accumulation Mechanism of SNst Contract

[sherlock-admin3]

JuggerNaut63

High

Reentrancy Exploit in Yield Accumulation Mechanism of SNst Contract

Summary

The drip function in the SNst is vulnerable to reentrancy attacks due to external calls to vat.suck and nstJoin.exit before updating critical state variables. This could allow an attacker to manipulate the state and drain funds.

Vulnerability Detail

1. Initial Call:
   • drip is called.
   • Calculates nChi and diff.
   • Calls vat.suck. https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/sdai/src/SNst.sol#L221
2. Reentrant Call:
   • If vat.suck or nstJoin.exit triggers a callback to drip, the function could be reentered before the state variables chi and rho are updated. https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/sdai/src/SNst.sol#L221-L222
   • This reentrant call would use the old values of chi and rho, leading to incorrect calculations and potential double spending of diff.

Impact

• Financial Loss
• State Manipulation

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/sdai/src/SNst.sol#L214-L229

Tool used

Manual Review

Recommendation

• Use OpenZeppelin's ReentrancyGuard to prevent reentrancy attacks.

• import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

• contract SNst is UUPSUpgradeable {

• contract SNst is UUPSUpgradeable, ReentrancyGuard {

• function drip() public returns (uint256 nChi) {

• function drip() public nonReentrant returns (uint256 nChi) { (uint256 chi, uint256 rho) = (chi, rho); uint256 diff; if (block.timestamp > rho_) { nChi = rpow(nsr, block.timestamp - rho) chi / RAY; uint256 totalSupply = totalSupply; diff = totalSupply_ nChi / RAY - totalSupply * chi / RAY; vat.suck(address(vow), address(this), diff * RAY); nstJoin.exit(address(this), diff); chi = uint192(nChi); // safe as nChi is limited to maxUint256/RAY (which is < maxUint192) rho = uint64(block.timestamp); } else { nChi = chi_; } emit Drip(nChi, diff); } }

• Ensure all state changes occur before any external calls.

  
  function drip() public returns (uint256 nChi) {
      (uint256 chi_, uint256 rho_) = (chi, rho);
      uint256 diff;
      if (block.timestamp > rho_) {
          nChi = _rpow(nsr, block.timestamp - rho_) * chi_ / RAY;
          uint256 totalSupply_ = totalSupply;
          diff = totalSupply_ * nChi / RAY - totalSupply_ * chi_ / RAY;
  
          // Update state before external calls

• chi = uint192(nChi);

• rho = uint64(block.timestamp);

      vat.suck(address(vow), address(this), diff * RAY);
      nstJoin.exit(address(this), diff);

• chi = uint192(nChi); // safe as nChi is limited to maxUint256/RAY (which is < maxUint192)

• rho = uint64(block.timestamp); } else { nChi = chi_; } emit Drip(nChi, diff); }

[telome]

Not an issue. vat and nstJoin do not call snst back.