code-423n4 / 2022-05-bunker-findings

1 stars 0 forks source link

Loss of NFTs via underflow on seizeAmount #114

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CToken.sol#L1090-L1092

Vulnerability details

Issue: seizeAmounts[] > seizeTokens, seizeTokens will underflow because of solidity 5. Underflow allows an illegitimate amount of NFTs to be seized.

Consequence: If a user's borrow health drops to allow legitimate liquidation, an attacker can seize their ERC-1155 collateral for free.

Proof of Concept

Mitigations

Mitigation: Use safeMath for seizeTokens -= seizeAmounts[i]

bunkerfinance-dev commented 2 years ago

While there is an underflow possibility that we should check for (it wouldn't hurt), I can't see how this can result in losing funds. repayAmount cannot be equal to 0 (see this line)

gzeoneth commented 2 years ago

Invalid because CNft is compiled with ^0.8.0 and seizeAmounts = [type(uint256).max, 1] would lead to overflow in totalAmount += amounts[i];

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L2-L3

pragma solidity ^0.8.0;

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L49-L54

        uint256 totalAmount = 0;
        for (uint256 i; i < length; ++i) {
            if (!is1155) {
                require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
            }
            totalAmount += amounts[i];