code-423n4 / 2024-07-karak-findings

0 stars 0 forks source link

Upgraded Q -> 2 from #22 [1723479041527] #100

Closed c4-judge closed 2 months ago

c4-judge commented 2 months ago

Judge has assessed an item in Issue #22 as 2 risk. The relevant finding follows:

[01] Protocol can fail to support fee-on-transfer tokens Description According to https://github.com/code-423n4/2024-07-karak?tab=readme-ov-file#erc20-token-behaviors-in-scope, fee-on-transfer tokens are in scope and should be supported. Calling the following slashAssets function further calls the handleSlashing function below that executes SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount) and then SafeTransferLib.safeTransfer(address(token), address(0), amount). When token is a fee-on-transfer token, after executing SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount), the SlashingHandler contract’s balance of such token would be less than amount because such token’s transfer fee is deducted, which then causes the execution of SafeTransferLib.safeTransfer(address(token), address(0), amount) to revert. As a result, the DSS fails to slash the operator’s vault when the vault’s underlying token is a fee-on-transfer token, and the protocol fails to support such fee-on-transfer token.

https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Vault.sol#L193-L205

function slashAssets(uint256 totalAssetsToSlash, address slashingHandler)
    external
    onlyCore
    returns (uint256 transferAmount)
{
    transferAmount = Math.min(totalAssets(), totalAssetsToSlash);

    // Approve to the handler and then call the handler which will draw the funds
    SafeTransferLib.safeApproveWithRetry(asset(), slashingHandler, transferAmount);
    ISlashingHandler(slashingHandler).handleSlashing(IERC20(asset()), transferAmount);
    ...
}

https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/SlashingHandler.sol#L52-L59

function handleSlashing(IERC20 token, uint256 amount) external {
    if (amount == 0) revert ZeroAmount();
    if (!_config().supportedAssets[token]) revert UnsupportedAsset();

    SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount);
    // Below is where custom logic for each asset lives
    SafeTransferLib.safeTransfer(address(token), address(0), amount);
}

Recommended Mitigation The handleSlashing function can be updated to transfer the actual token amount received by the SlashingHandler contract through the SafeTransferLib.safeTransferFrom function call when calling the SafeTransferLib.safeTransfer function.

c4-judge commented 2 months ago

MiloTruck marked the issue as satisfactory

c4-judge commented 2 months ago

MiloTruck marked the issue as duplicate of #93

c4-judge commented 1 month ago

MiloTruck marked the issue as unsatisfactory: Out of scope