`fee`s transferred affected at `add`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/d7843c5d7ab731ba959cdb1389ba6bd5a2f5bbd3/src/CidNFT.sol#L189-L194

Vulnerability details

fees transferred affected at CidNFT#add()

Summary

cidFee is 0 for some values and second transfer of fee amount will be a little bigger than it should.

Vulnerability Detail

CID_FEE_BPS is a constant with value 1_000 if subprotocolFee > 0 and subprotocolFee < 10, when calculating cifFee, result will be 0 due to solidity rounding.

uint96 subprotocolFee = subprotocolData.fee;
if (subprotocolFee != 0) {
    uint256 cidFee = (subprotocolFee * CID_FEE_BPS) / 10_000;
    SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, cidFee);
    SafeTransferLib.safeTransferFrom(note, msg.sender, subprotocolOwner, subprotocolFee - cidFee);
}

This would affect following 2 transfers, making bigger the second transfer and 0 the first transfer

SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, cidFee);
SafeTransferLib.safeTransferFrom(note, msg.sender, subprotocolOwner, subprotocolFee - cidFee);

Impact

Different amounts of fee transferred to cidFeeWallet and subprotocolOwner than they should

Code Snippet

• CidNFT.sol#L189-L194

Tool used

Manual Review

Recommendation

Revert with values in the range of:

subprotocolFee > 0 and subprotocolFee < 10

or take into account rounding for the operation

[c4-judge]

berndartmueller marked the issue as duplicate of #14

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c