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

2 stars 1 forks source link

updating the registry hash after making state changes can lead to inconsistent state if the registry update fails #275

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#L175 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L176 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L179-L180

Vulnerability details

Impact

This could allow the old owner to still act as the delegate, since the registry hash has not been updated. Or conversely, the new owner's balance is increased but the old hash means they cannot act as the delegate

Proof of Concept

The Transfer event is emitted and balances updated before writing the new registry hash. If RegistryHelpers.transferERC721 fails, the contract state will be inconsistent - the Transfer event and balances change will have occurred, but the registry hash will still point to the old delegation. This could allow the old owner to still act as the delegate, since the registry hash has not been updated. Or conversely, the new owner's balance is increased but the old hash means they cannot act as the delegate

Tools Used

Manual

Recommended Mitigation Steps

The registry hash should be updated first before making any state changes

Assessed type

Other

c4-judge commented 12 months ago

GalloDaSballo marked the issue as unsatisfactory: Invalid