c4-submissions commented 10 months ago

Lines of code

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

Vulnerability details

Impact

Rebasing tokens will have incorrect withdrawal amounts

Proof of Concept

There are some ERC20's which rebase such as Ampleforth. This means that the underlying supply of the token changes between the time create is called to create the principal and delegate token to the time where the underlying is withdrawn. Specifically, the underlyingAmount within this codeblock in the withdraw function would be incorrect:

        } else if (delegationType == IDelegateRegistry.DelegationType.ERC20) {
            uint256 erc20UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
            StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
            RegistryHelpers.decrementERC20(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc20UnderlyingAmount, underlyingRights);
            StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
            SafeERC20.safeTransfer(IERC20(underlyingContract), msg.sender, erc20UnderlyingAmount);

This means that the wrong amount of tokens would be withdrawn causing loss of funds, or insolvency within that particular token.

Tools Used

Manual review

Recommended Mitigation Steps

It would be best to remove rebasing tokens as an acceptable option for Delegate as it would be difficult to get the accounting correct for all different types of rebasing tokens.

Assessed type

ERC20

c4-judge commented 10 months ago

GalloDaSballo marked the issue as duplicate of #257

c4-judge commented 9 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)