TransferFundsOnBuybackFacet Manual Review Findings

TFO-01M: Incorrect State Transition

Type	Severity	Location
Logical Fault		TransferFundsOnBuybackFacet.sol:L32

Description:

The referenced state transition is considered insecure as it will attempt to increase the state by one which might not always advanced to the desired post-buyback state.

Impact:

The buy-back mechanism is incompatible with the type(uint256).max state due to an overflow error and generally relies on an assumption that can be trivially breached.

Example:

Copy
23function doBuyback(uint256 campaignId, address account, uint256 buyBackAmount) external onlyInternalDelegateCall {
24    FungibleAndSemiFungiblePurchaseStorage.IdInfo memory info = FungibleAndSemiFungiblePurchaseStorage.layout().infoForId[
25        campaignId
26    ];
27    IERC20(info.fundingCurrency).safeTransferFrom(account, address(this), buyBackAmount);
28
29    // The change of state here is extremely important, maybe it should be a different facet.
30    // The importance comes from the fact that if state does not change, buyback can happen over and over again!
31    uint256 currentState = IStateFacet(address(this)).getStateOfId(campaignId);
32    IStateFacet(address(this)).changeState(campaignId, currentState, currentState + 1); // Is that always the case? TBD...
33
34    emit DoBuybackExecuted(campaignId, account, buyBackAmount);
35}

Recommendation:

We advise the system to reserve the 8 top bits of the state IDs by restricting state configurations to the uint248 data type, permitting special states such as buy-backs and post-receives to be represented as the maximum value of the uint256 data type and downward through constant declarations.

Alleviation (71cda4ccfdcfa25fb96a4565f1f8143b350dd246):

The system utilizes a storage-configured post-buyback state in the latest implementation, permitting the Evergon Labs team to define a specific state that is considered to represent the "post-buyback" state of a campaign thus addressing this exhibit.

TFO-02M: Inexistent Release of Bought NFT

Type	Severity	Location
Logical Fault		TransferFundsOnBuybackFacet.sol:L23-L35

Description:

The TransferFundsOnBuybackFacet::doBuyback mechanism will not actually unlock the underlying NFT or release it to the new account that has paid for it which we consider invalid.

Impact:

A user who buys back a fractionalized wrapper NFT will not actually acquire it resulting in fund loss as the NFT will also become inaccessible by its original owner due to the state advancement.

Example:

Copy
23function doBuyback(uint256 campaignId, address account, uint256 buyBackAmount) external onlyInternalDelegateCall {
24    FungibleAndSemiFungiblePurchaseStorage.IdInfo memory info = FungibleAndSemiFungiblePurchaseStorage.layout().infoForId[
25        campaignId
26    ];
27    IERC20(info.fundingCurrency).safeTransferFrom(account, address(this), buyBackAmount);
28
29    // The change of state here is extremely important, maybe it should be a different facet.
30    // The importance comes from the fact that if state does not change, buyback can happen over and over again!
31    uint256 currentState = IStateFacet(address(this)).getStateOfId(campaignId);
32    IStateFacet(address(this)).changeState(campaignId, currentState, currentState + 1); // Is that always the case? TBD...
33
34    emit DoBuybackExecuted(campaignId, account, buyBackAmount);
35}

Recommendation:

We advise the TransferFundsOnBuybackFacet::doBuyback function to release the underlying wrapper NFT represented by the fractionalized EIP-1155 NFT to its caller, ensuring that the buyback mechanism functions as intended.

Alleviation (71cda4ccfdcfa25fb96a4565f1f8143b350dd246):

The code was updated by introducing an IFraction::forceUnlcok function call, ensuring that the NFT is forcefully unlocked after the buyback occurs to the relevant account, alleviating this exhibit as a result.