search

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

2 stars 1 forks source link

Emitting the Transfer event before updating the registry can enable reentrancy attacks. #262

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

It breaks the expected ownership and transfer logic in DelegateToken.

Proof of Concept

  1. The DelegateToken contract emits the Transfer event first before updating the registry:https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L172 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L180
  2. Another contract is listening for the Transfer event and makes a call back into DelegateToken as part of its callback logic, such as calling transferFrom again.
  3. Since the registry has not been updated yet in DelegateToken, the reentrancy call will pass the ownership checks and allow the transfer to go through again. This allows an attacker to drain tokens by calling transferFrom multiple times before the registry is updated.

Tools Used

Manual

Recommended Mitigation Steps

The registry should be updated first before emitting the event

Assessed type

Other

c4-judge commented 12 months ago

GalloDaSballo marked the issue as unsatisfactory: Invalid