Potential for reentrancy on `USDMPegRecovery.sol:withdraw()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L110-L128

Vulnerability details

Impact

Re-entrancy

Proof of Concept

File: USDMPegRecovery.sol
110:     function withdraw(Liquidity calldata _withdrawal) external {
111:         Liquidity memory total = totalLiquidity;
112:         Liquidity memory user = userLiquidity[msg.sender];
113:         if(_withdrawal.usdm > 0) {
114:             require(unlockable, "!unlock usdm");
115:             usdm.safeTransfer(msg.sender, uint256(_withdrawal.usdm));
116:             total.usdm -= _withdrawal.usdm;
117:             user.usdm -= _withdrawal.usdm;
118:         }
119: 
120:         if(_withdrawal.pool3 > 0) {
121:             pool3.safeTransfer(msg.sender, uint256(_withdrawal.pool3));
122:             total.pool3 -= _withdrawal.pool3;
123:             user.pool3 -= _withdrawal.pool3;
124:         }
125:         totalLiquidity = total;
126:         userLiquidity[msg.sender] = user;
127:         emit Withdraw(msg.sender, _withdrawal);
128:     }

The code is applying an anti-pattern:

• No Check Effects Interactions pattern (updating state variables AFTER the transfer)
• No access control (no modifiers)
• No lock
• No ReentrancyGuard (no modifiers)

Also, this is a user-controlled input (potential for sending a malicious argument from a malicious contract (for msg.sender))

Tools Used

VS Code

Recommended Mitigation Steps

Use one of the mitigations listed above.

[r2moon]

usdm and pool3(crv) are simple erc20 token which don't lead re-entrancy.

[GalloDaSballo]

Agree with the sponsor, the tokens don't have hooks as such no reEntrancy can happen