Open code423n4 opened 1 year ago
GalloDaSballo marked the issue as primary issue
This is even better (POC test vs POC log)
That will happen only with the first opened position until _handleCloseFees()
is called.
Valid but I think it should be low risk as it will mostly not affect anyone.
Also the funds that are not distributed will be distributed later because of gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
so no funds will be lost.
TriHaz marked the issue as disagree with severity
TriHaz marked the issue as sponsor confirmed
The warden has shown how, due to a lack of approvals, the rewards earned until the first call to _handleCloseFees
We also know that _handleDeposit
will burn the balance of tigAsset
that is unused.
The risk however, is limited to the first (one or) few users, for this reason I believe that Medium Severity is more appropriate.
Adding an approval on deployment or before calling distribute
should help mitigate
GalloDaSballo changed the severity to 2 (Med Risk)
GalloDaSballo marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L689-L750 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L287-L294
Vulnerability details
Impact
Calling the following
Trading._handleOpenFees
function does not approve theGovNFT
contract for spending any of theTrading
contract's_tigAsset
balance, which is unlike calling theTrading._handleCloseFees
function below that executesIStable(_tigAsset).approve(address(gov), type(uint).max)
. Due to this lack of approval, when calling theTrading._handleOpenFees
function without theTrading._handleCloseFees
function being called for the same_tigAsset
beforehand, theGovNFT.distribute
function's execution ofIERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount)
in thetry...catch...
block will not transfer any_tigAsset
amount as the trade's DAO fees to theGovNFT
contract. In this case, although the Governance NFT holder, whose NFT was minted before theTrading._handleOpenFees
function is called, deserves the rewards from the DAO fees generated by the trade, this holder does not have any pending rewards after suchTrading._handleOpenFees
function call because none of the DAO fees were transferred to theGovNFT
contract. Hence, this Governance NFT holder loses the rewards that she or he is entitled to.https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L689-L750
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L287-L294
Proof of Concept
Functions like
Trading.initiateMarketOrder
further call theTrading._handleOpenFees
function so this POC uses theTrading.initiateMarketOrder
function.Please add the following test in the
Signature verification
describe
block intest\07.Trading.js
. This test will pass to demonstrate the described scenario. Please see the comments in this test for more details.Furthermore, as a suggested mitigation, please add
IStable(_tigAsset).approve(address(gov), type(uint).max);
in the_handleOpenFees
function as follows in line 749 ofcontracts\Trading.sol
.Then, as a comparison, the following test can be added in the
Signature verification
describe
block intest\07.Trading.js
. This test will pass to demonstrate that the Governance NFT holder's pending rewards is no longer 0 after implementing the suggested mitigation. Please see the comments in this test for more details.Tools Used
VSCode
Recommended Mitigation Steps
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L749 can be updated to the following code.