search

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

1 stars 0 forks source link

CNft.sol - revert inside safeTransferFrom will break composability & standard behaviour #93

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

The function safeTransferFrom is a standard interface in ERC1155, and its expected to succeed if all the parametes are valid, and revert on error, which is not the case here so its a deviation.

Refer to the EIP-1155 safeTransferFrom rules:

MUST revert if _to is the zero address. MUST revert if balance of holder for token _id is lower than the _value sent to the recipient. MUST revert on any other error.

There is no loss of assets, but the assets or tokens and CNft contract can be unusable by other protocols, and likelihood & impact of this issue is high.

Impact

If other protocols want to integrate CNft, then in that case just for CNft Contract / tokens, they have to take exception and use safeBatchTransferFrom, instead of safeTransferFrom. If they dont take care of this exception, then their protocol functions will fail while using CNft, even if valid values are given.

Proof of Concept

Contract : CNft.sol Function : safeTransferFrom

Line 204 revert("CNFT: Use safeBatchTransferFrom instead");

Recommended Mitigation Steps

Instead of revert, call function safeBatchTransferFrom with 1 item in the array, e.g.,

safeBatchTransferFrom(from, to, [id], [amount], data)

bunkerfinance-dev commented 2 years ago

We can fix this, but we do not feel like this is high severity at all.

gzeoneth commented 2 years ago

I think this is a Med Risk issue as it impact the function of the protocol.