No check for return value with transferfrom function could lead to incorrect assumptions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L148
https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L160
https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L170

Vulnerability details

Bug Description

In ERC20MultiDelegate contract, the ERC20 transferfrom function is called if a user wishes to process a new delegation through _processDelegation, get back his token and voting power with _reimburse, or create a new delegation with createProxyDelegatorAndTransfer.

In case the ERC20 token used within ERC20MultiDelegate contract correctly reverts if any issue occurs during transfer, there won't be any problem related to that. But if an ERC20 token that doesn't properly revert in case of transfer failure is used in the ERC20MultiDelegate contract, this could lead to transactions being successful while transferfrom didn't actually do any transfer. Therefore, a user could think his delegation changed while it didn't. As the sponsors told us ERC20MultiDelegate could be used for any ERC20 token, this might be something to consider.

Impact

using ERC20MultiDelegate contract to delegate voting power of an ERC20 token that doesn't properly reverts in case of transfer failure would break individual assumptions, because transaction will be successful although delegation didn't change.

Tools Used

Manual

Recommended Mitigation Steps

Using safe ERC20 or checking for return values could mitigate the risk involved by the issue.

Assessed type

Token-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #91

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #90

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-b