Distribute is open to rewards manipulation

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L225 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L240 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L185

Vulnerability details

Impact

The distribute() function is prone to manipulation by the first depositor if the totalShares is low, since the result of transferFrom() in distribute() is not checked. This can happen if a malicious user calls createLock() whereby shares = 1 then calls distribute() with a large amount.

Proof of Concept

1) User calls createLock() to create bond with _amount _period == 365: then the number of shares will be equal to 0. For example, _amount = 31 wei and period = 12. 2) User then calls distribute() with a very large _amount, the transferFrom() will fail but is not checked, so this will increase accRewardsPerShare by _amount 1e18 per call via https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L225
3) User then calls claim() in Lock which will call claim() in BondNFT, which triggers idToBond which will increment pending value to a very large value: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L240-L241 4) Amount transferred to user is based on the above large pending value: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L185

Tools Used

None

Recommended Mitigation Steps

Add a check on the transferFrom() result and also add a check on the number of shares created.

[GalloDaSballo]

Would benefit by fuzzing and further work, but I think it's a valid concern

[GalloDaSballo]

This is chaining the safeTransfer which is OOS, but I think because that's used as a component, I won't close it and will review

[TriHaz]

User then calls distribute() with a very large _amount, the transferFrom() will fail but is not checked, so this will increase accRewardsPerShare by _amount * 1e18 per call via https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L225

That will revert.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

@TriHaz transferFrom is unchecked, so for tokens that do not revert on transfer, the call will not revert (it would return false, but that return value is not checked)

[GalloDaSballo]

In contrast to other reports, in this one, the Warden has shown how, because of an unchecked transfer, rewards could be stolen without performing any deposit.

Because this shows an attack chaining, which relies on external conditions, I think the finding to be valid and of Medium Severity

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

After an additional check, I realized that the tigAsset is just a OZ ERC20 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableToken.sol#L4-L5

import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";

Meaning that this finding is invalid

I also had multiple other doubts about this finding, but because of the lack of detail am closing as invalid

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid