code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

Decrementing the balance before transferring could lead to incorrect state if the transfer fails. #276

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L360 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L369

Vulnerability details

Impact

If a transfer were to fail, the contract balance would be incorrect, showing less tokens than it actually holds. This could lead to errors or exploits down the line if relying on an inaccurate balance

Proof of Concept

  1. In the withdraw() function, it first decrements the balance: https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L360
  2. Then it transfers the token: https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L369 If the transferFrom() call were to fail and revert for some reason, the balance would already have been decremented, leaving it in an incorrect state.

Tools Used

Manual

Recommended Mitigation Steps

A better approach is to transfer first, then decrement if successful

Assessed type

Other

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid