checkTransfer is always true#

Informational

In View.sol there is a checkTransfer function that returns a bool value. However, this value is always true, otherwise the function reverts. In CanvasCollection.sol there are several instances where checkTransfer is called. Each time it is done in an if statement to verify that the return value is true. However since the result is always true (otherwise it reverts), the check is unnecessary.

Example#

If we look at this code form View.sol, we can see that checkTransfer returns !canvas.soulbound. But that will always result in true, because we can see few lines above, that if canvas.soulbound is true, revert NonTransferable() will be executed.

function checkTransfer(
    address collectionAddress,
    address from,
    address to,
    uint256 tokenId
  ) external returns (
    bool
  ) {
    Schema.Storage storage ds = canvasStorage();

    uint256 canvasId;
    if (collectionAddress == ds.contractInfo.canvasOneAddress) canvasId = ds.canvasOne[tokenId];
    else if (collectionAddress == ds.contractInfo.canvasOneCuratedAddress) canvasId = ds.canvasOneCurated[tokenId];
    else canvasId = ds.collectionToCanvas[collectionAddress];

    Schema.Canvas storage canvas = ds.canvas[canvasId];
    if (!ds.canvasSystem[canvasId].approved) revert InvalidToken();
    if (canvas.soulbound) revert NonTransferable();

    emit Transfer(collectionAddress, from, to, tokenId);
    return !canvas.soulbound;
  }

Taking a look at code of CanvasCollection.sol, we can see the if statement that is unnecessary.

if (ICanvas(canvasAddress).checkTransfer(from, to, tokenId)) {
  super.transferFrom(from, to, tokenId);
}

Recommendation#

In View.sol remove the return statement from checkTransfer, because there is no upside to returning a boolean that is always true.

function checkTransfer(
    address collectionAddress,
    address from,
    address to,
    uint256 tokenId
  ) external {
    Schema.Storage storage ds = canvasStorage();

    uint256 canvasId;
    if (collectionAddress == ds.contractInfo.canvasOneAddress) canvasId = ds.canvasOne[tokenId];
    else if (collectionAddress == ds.contractInfo.canvasOneCuratedAddress) canvasId = ds.canvasOneCurated[tokenId];
    else canvasId = ds.collectionToCanvas[collectionAddress];

    Schema.Canvas storage canvas = ds.canvas[canvasId];
    if (!ds.canvasSystem[canvasId].approved) revert InvalidToken();
    if (canvas.soulbound) revert NonTransferable();

    emit Transfer(collectionAddress, from, to, tokenId);
  }

Remove the if statements in CanvasCollection.sol and just call the checkTransfer function.

ICanvas(canvasAddress).checkTransfer(from, to, tokenId)
super.transferFrom(from, to, tokenId);