Distribute has transfer of assets that should use a contract before and after balance

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L216 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L217-L226

Vulnerability details

Impact

The distribute function uses transferFrom() without monitoring balances, leading to rewards accounting being wrong

Proof of Concept

In BondNFT, distribute() is open to any caller. Since the transfer of assets https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L216 occurs before the rewards accounting gets updated: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L217-L226 and since the transferFrom() call is not guaranteed to transfer _amount (can fail silently or be less), then contract balance should be used before and after the transferFrom() to determine the actual amount to use in updating rewards. Additionally, if the actual amount transferred is actually 0 or fails, then the entire unchecked block should not be updated, therefore a require should be used to check that.

Tools Used

None

Recommended Mitigation Steps

Add in the lines (where … is existing code):

uint256 balanceBefore = IERC20(_tigAsset).balanceOf(address(this)); transferFrom(...); uint256 balanceAfter = IERC20(_tigAsset).balanceOf(address(this)); require(balanceAfter - balanceBefore > 0, “not updating rewards”); … accRewardsPerShare[_tigAsset][aEpoch] += (balanceAfter - balanceBefore) * 1e18 / totalShares[_tigAsset]; …

[GalloDaSballo]

Effectively same as malicious ERC20 / Fee On Transfer

Technically reduced because the token has got to be allowedAsset

[c4-judge]

GalloDaSballo marked the issue as duplicate of #522

[GalloDaSballo]

Marking as dup as FeeOnTransfer

using safeTransfer is OOS

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[GalloDaSballo]

L

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/607