Tigris Trade contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L168-L184 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L235-L243

Vulnerability details

Impact

In BondNFT.sol#claim(), accRewardsPerShare[][] is amended to reflect the expired shares. But the current way introduces other issues:

• All the rewards could be drained.
• accRewardsPerShare might be inaccurate.

Proof of Concept

The rationale behind the unchecked block below seems to take into account the shares of reward of the expired bond. However, the current way is flawed.

File: contracts/BondNFT.sol
168:     function claim(
169:         uint _id,
170:         address _claimer
171:     ) public onlyManager() returns(uint amount, address tigAsset) {
    
176:         unchecked {
177:             if (bond.expired) {
178:                 uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
179:                 if (totalShares[bond.asset] > 0) {
180:                     accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
181:                 }
182:             }
183:             bondPaid[_id][bond.asset] += amount;
184:         }

Moreover, this treatment could be abused to steal all the rewards in the contract.

Assuming, on day 0, epoch[tigAsset] is 100, accRewardsPerShare[tigAsset][100] is 200. totalShares[tigAsset] is 300. (decimal calculation will be omitted for simplicity) The malicious user will do the following:

• have 2 BondNFT with the same tigAsset, both have shares of 100, and id 1001 and 1002. 1001 expires 7 days later, 1002 expires 14 days later. Now totalShares[tigAsset] is 500.
• on day 6, accRewardsPerShare[tigAsset][106] is 300, which will be used for 1001 rewards calculation.
• on day 9, accRewardsPerShare[tigAsset][109] is 400, due to new distribution.
• on day 9, 1001 has expired for 2 days. The user will call Lock.sol#claim(1001) to have the reward for 1001. _pendingDelta will be 100 * 400 - 100 * 300 = 10000. According to line 180, accRewardsPerShare[tigAsset][109] will be increased by 10000 / 500 = 20, change from 400 to 420.
• The user then calls Lock.sol#claim(1001) again. bond.pending will be 0, but the unchecked block will be run one more time, _pendingDelta will be 100 * 420 - 100 * 300 = 12000. accRewardsPerShare[tigAsset][109] will be increased by 12000 / 500 = 24, change from 420 to 444.
• the user will repeat until accRewardsPerShare[tigAsset][109] is large enough. Then using bond 1002, to claim the rewards. With a large accRewardsPerShare[tigAsset][109], all the reward balance can be taken.

The claimable reward is stored in bond.pending, which refers to the latest accRewardsPerShare[tigAsset][epoch[tigAsset]]

235:     function idToBond(uint256 _id) public view returns (Bond memory bond) {
236:         bond = _idToBond[_id];
237:         bond.owner = ownerOf(_id);
238:         bond.expired = bond.expireEpoch <= epoch[bond.asset] ? true : false;
239:         unchecked {
240:             uint _accRewardsPerShare = accRewardsPerShare[bond.asset][bond.expired ? bond.expireEpoch-1 : epoch[bond.asset]];
241:             bond.pending = bond.shares * _accRewardsPerShare / 1e18 - bondPaid[_id][bond.asset];
242:         }
243:     }

Tools Used

Manual analysis.

Recommended Mitigation Steps

To take into account the impact of expired bond on accRewardsPerShare[][], the key is on totalShares[tigAsset]. Below is the way accRewardsPerShare[][] being updated for now. But the totalShares[tigAsset] used here also includes the expired shares.

211:     function distribute() external {

225:             accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];

The suggestion is to refactor totalShares[tigAsset] in line 225, to subtract the expired portion at the beginning. Otherwise the whole accRewardsPerShare[][] accounting is error prone.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L44-L72

Vulnerability details

Impact

Currently the exchange rate for tigUSD and other stable coin is hardcoded as 1:1. According to the doc

They are redeemable 1:1 for any asset in its respective StableVault

However, mainstream stable coins prices did fluctuates, USDT/USDC/DAI all could go up and down. If the strict 1:1 exchange rate is applied, sometimes there would be arbitrage opportunity and the contract could suffer fund loss.

Proof of Concept

From the historical prices of some stable coins, it can be seen that price could deviate from 1, and different stable coins are not in phase with each other, which leaves potential room to take advantage of tigUSD vault.

• USDT in 11/21/2018, the price falls to as low as 0.9734, and in 12/20/2018 rises to as high as 1.024.

• USDC in 03/19/2020, the price falls to as low as 0.9709, and in 01/31/2020 rises to as high as 1.0436.

• DAI in 05/16/2020, the price falls to as low as 0.9846, and in 09/11/2020 rises to as high as 1.0423.

Considering the non-transparency of USDT, regulatory risks of USDC and other defi protocols, unforeseen attack causing contract being compromised, etc. Not to mention severe events for luna/UST. The prices of stable coins are not guaranteed to keep stay around 1 USD.

The exchange rate for tigUSD and other stable coins are hardcoded as 1:1 in StableVault.sol.

File: contracts/StableVault.sol
44:     function deposit(address _token, uint256 _amount) public {
45:         require(allowed[_token], "Token not listed");
46:         IERC20(_token).transferFrom(_msgSender(), address(this), _amount);
47:         IERC20Mintable(stable).mintFor(
48:             _msgSender(),
49:             _amount*(10**(18-IERC20Mintable(_token).decimals()))
50:         );
51:     }

53:     function depositWithPermit(address _token, uint256 _amount, uint256 _deadline, bool _permitMax, uint8 v, bytes32 r, bytes32 s) external {
54:         uint _toAllow = _amount;
55:         if (_permitMax) _toAllow = type(uint).max;
56:         ERC20Permit(_token).permit(_msgSender(), address(this), _toAllow, _deadline, v, r, s);
57:         deposit(_token, _amount);
58:     }


65:     function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
66:         IERC20Mintable(stable).burnFrom(_msgSender(), _amount);
67:         _output = _amount/10**(18-IERC20Mintable(_token).decimals());
68:         IERC20(_token).transfer(
69:             _msgSender(),
70:             _output
71:         );
72:     }

If some day USDT prices falls to 0.99, but DAI still 1.00. Users can deposit with USDT and withdraw with DAI. Until all the balance of the vault is taken. The fund loss due to exchange difference will be left for the contract.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use real time price feed from reliable oracles for stable coin vault, just like for trading contracts, verify the prices before deposit()/withdraw().

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L110-L120 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L275-L280 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L357-L360 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L211-L215

Vulnerability details

Impact

BondNFT has its allowedAsset[] for allowed assets. But from Lock.sol#claimGovFees(), GovNFT will claim any asset rewards no matter it is allowed in BondNFT or not. So some asset not in BondNFT allowedAsset[] could possibly be collected from GovNFT claim(), in this case the corresponding rewards will be lost and locked in the contract.

Proof of Concept

Since the assets could change over time, the admin sometimes need to maintain the assets[] and allowedAsset[]. But for GovNFT, any asset can be claimed as rewards.

File: contracts/Lock.sol
110:     function claimGovFees() public {
111:         address[] memory assets = bondNFT.getAssets();
112: 
113:         for (uint i=0; i < assets.length; i++) {
114:             uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
115:             IGovNFT(govNFT).claim(assets[i]);
116:             uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
117:             IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
118:             bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
119:         }
120:     }

File: contracts/GovNFT.sol
275:     function claim(address _tigAsset) external {
276:         address _msgsender = _msgSender();
277:         uint256 amount = pending(_msgsender, _tigAsset);
278:         userPaid[_msgsender][_tigAsset] += amount;
279:         IERC20(_tigAsset).transfer(_msgsender, amount);
280:     }

In BondNFT, some asset could be set as false. Then the BondNFT.sol#distribute() will silently return and do nothing.

File: contracts/BondNFT.sol
357:     function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
358:         require(assets[assetsIndex[_asset]] == _asset, "Not added");
359:         allowedAsset[_asset] = _bool;
360:     }

211:     function distribute(
212:         address _tigAsset,
213:         uint _amount
214:     ) external {
215:         if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;

In this situation, although some asset might be claimed by GovNFT, and the balanceBefore and balanceAfter were changed, this part of rewards will be ignored in BondNFT.sol#distribute(). And be locked in the contract.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• In Lock.sol#claimGovFees(), Skip the disallowed assets.

File: contracts/Lock.sol
    function claimGovFees() public {
        address[] memory assets = bondNFT.getAssets();

        for (uint i=0; i < assets.length; i++) {
            if (bondNFT.allowedAsset(assets[i])) {
                uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
                IGovNFT(govNFT).claim(assets[i]);
                uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
                IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
                bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
            }
        }
    }

• In GovNFT, also enforce some allowedAssets list, to be synchronized with BondNFT.
• Use some admin function to handle the disallowed assets in GovNFT.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L652-L653 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L643-L648 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L53-L58

Vulnerability details

Impact

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals. In Trading.sol#_handleDeposit(), this could cause some issue as approve(0) is not called before setting new approval value. If in some cases, the approval value of USDT fo the stable vault is changed, all the calls to _handleDeposit() will fail.

Proof of Concept

If USDT is used as the margin asset, and the approval value is not 0 in some situations, the following approve() in deposit function will revert.

File: contracts/Trading.sol
652:             IERC20(_marginAsset).approve(_stableVault, type(uint).max);
653:             IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);

In addition, the permit() function also call _approve().

File: contracts/StableVault.sol
53:     function depositWithPermit(address _token, uint256 _amount, uint256 _deadline, bool _permitMax, uint8 v, bytes32 r, bytes32 s) external {
54:         uint _toAllow = _amount;
55:         if (_permitMax) _toAllow = type(uint).max;
56:         ERC20Permit(_token).permit(_msgSender(), address(this), _toAllow, _deadline, v, r, s);
57:         deposit(_token, _amount);
58:     }

File: contracts/Trading.sol
643:     function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
644:         IStable tigAsset = IStable(_tigAsset);
645:         if (_tigAsset != _marginAsset) {
646:             if (_permitData.usePermit) {
647:                 ERC20Permit(_marginAsset).permit(_trader, address(this), _permitData.amount, _permitData.deadline, _permitData.v, _permitData.r, _permitData.s);
648:             }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use OpenZeppelin's SafeERC20's safeTransfer() instead.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L847-L849

Vulnerability details

Impact

The position is implemented as NFT. If a position NFT owner approves another user for some position or set other user as an operator for all their positions (rather than just for a given token), the validation check in Trading.sol#_checkOwner(uint _id, address _trader) will fail.

Proof of Concept

The position is implemented as an ERC721, which has two ways to approve another user:

• Approve them to take actions with a given token (approve())
• Approve them as an "operator" for all owned tokens (setApprovalForAll())

However, when the _checkOwner(_id, _trader) function checks that the token is owned by _trader, it does not accept those who are approved or set as operators.

File: contracts/Trading.sol
847:     function _checkOwner(uint _id, address _trader) internal view {
848:         if (position.ownerOf(_id) != _trader) revert("2"); //NotPositionOwner   
849:     }

Approved operators of position NFTs will be rejected from taking actions with those tokens.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Include an additional check to confirm whether the _trader is approved or as an operator on the position NFT:

    function _checkOwner(uint _id, address _trader) internal view {
        address _owner = position.ownerOf(_id)
        if (_owner != _trader && position.getApproved(_id) != _trader && position.isapprovedForAll(_owner) != _trader) revert("2"); //NotPositionOwner   
    }

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L300-L305 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L50-L53 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L64-L67 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L76-L78 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L89-L95

Vulnerability details

Impact

It's possible to render the GovNFT.sol contract inoperable due to DoS. All the mint()/_bridgeMint()/_burn()/_transfer() could fail to function.

Proof of Concept

There is no upper limit for the assets[] array, and there is no way to decrease the length of the array. If the length of it grow too large, it is possible that in the loop when iterating all the elements, the gas required exceeds the block gas limit and cause DoS.

There is no limit on the size on the array when add new asset into it. But no way to remove items.

File: contracts/GovNFT.sol
300:     function addAsset(address _asset) external onlyOwner {
301:         require(assets.length == 0 || assets[assetsIndex[_asset]] != _asset, "Already added");
302:         assetsIndex[_asset] = assets.length;
303:         assets.push(_asset);
304:         _allowedAsset[_asset] = true;
305:     }

In mint()/_bridgeMint()/_burn()/_transfer(), the entire assets[] array will be iterated. So if the size become too large after a long time, the contract could be inoperable.

50:     function _mint(address to, uint256 tokenId) internal override {

53:         for (uint i=0; i<assetsLength(); i++) {


64:     function _bridgeMint(address to, uint256 tokenId) public {

67:         for (uint i=0; i<assetsLength(); i++) {


76:     function _burn(uint256 tokenId) internal override {

78:         for (uint i=0; i<assetsLength(); i++) {

89:     function _transfer() internal override {

95:         for (uint i=0; i<assetsLength(); i++) {

Tools Used

Manual analysis.

Recommended Mitigation Steps

Set a maximum length for assets[] array.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L177-L183 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L235-L242

Vulnerability details

Impact

In BondNFT.sol#claim(), accRewardsPerShare[][] is amended to reflect the expired shares. But only accRewardsPerShare[bond.asset][epoch[bond.asset]] is updated. All the epochs between bond.expireEpoch-1 and epoch[bond.asset] are missed. However, some users claimable rewards calculation could be based on the missed epochs. As a result, the impact might be:

• accRewardsPerShare be inaccurate for the epochs in between.
• some users could lose reward due to wrong accRewardsPerShare, some users might receive undeserved rewards.
• some rewards will be locked in the contract.

Proof of Concept

The rationale behind the unchecked block below seems to take into account the shares of reward of the expired bond. However, if only update the latest epoch data, the epochs in between could have errors and lead to loss of other users.

File: contracts/BondNFT.sol
168:     function claim(
169:         uint _id,
170:         address _claimer
171:     ) public onlyManager() returns(uint amount, address tigAsset) {
    
177:             if (bond.expired) {
178:                 uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
179:                 if (totalShares[bond.asset] > 0) {
180:                     accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
181:                 }
182:             }
183:             bondPaid[_id][bond.asset] += amount;

Users can claim rewards up to the expiry time, based on accRewardsPerShare[tigAsset][bond.expireEpoch-1]:

235:     function idToBond(uint256 _id) public view returns (Bond memory bond) {
    
238:         bond.expired = bond.expireEpoch <= epoch[bond.asset] ? true : false;
239:         unchecked {
240:             uint _accRewardsPerShare = accRewardsPerShare[bond.asset][bond.expired ? bond.expireEpoch-1 : epoch[bond.asset]];
241:             bond.pending = bond.shares * _accRewardsPerShare / 1e18 - bondPaid[_id][bond.asset];

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L168-L185 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L235-L243

Vulnerability details

Impact

• some users reward could be diluted due to unreleased bond
• some reward will be locked forever, not able be claimed by anyone

Proof of Concept

Imagine, only 2 users, Alice has shares 9,900 for 6 month, Bob has shares of 100 for 12 month.

• during the beginning 6 month, total rewards of 1000 tigUSD is distributed, Alice gets 990, Bob get 10.
• after 6 month, within 7 epochs, Alice does not call release().
• within the 7 epochs, another 100 tigUSD reward is distributed.
  • according to the rule, Alice can get nothing since the bond has expired
  • Bob can only get 1 tigUSD, because he has only 1% of the total share

In this case, the remaining 99 tigUSD will be locked in the BondNFT.sol contract.

The issue is that totalSHares does not exclude the expired shares. These portion will only dilute the reward for existing shares. And the other part belongs to no one, so can only locked in the contract.

File: contracts/BondNFT.sol
211:     function distribute(
212:         address _tigAsset,
213:         uint _amount
214:     ) external {

225:             accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];

The unreleased shares will not get the reward due to the following. These portion only dilute the rewards of existing shares in distribute().

168:     function claim(
169:         uint _id,
170:         address _claimer
171:     ) public onlyManager() returns(uint amount, address tigAsset) {
172:         Bond memory bond = idToBond(_id);

174:         amount = bond.pending;

185:         IERC20(tigAsset).transfer(manager, amount);


235:     function idToBond(uint256 _id) public view returns (Bond memory bond) {
236:         bond = _idToBond[_id];
237:         bond.owner = ownerOf(_id);
238:         bond.expired = bond.expireEpoch <= epoch[bond.asset] ? true : false;
239:         unchecked {
240:             uint _accRewardsPerShare = accRewardsPerShare[bond.asset][bond.expired ? bond.expireEpoch-1 : epoch[bond.asset]];
241:             bond.pending = bond.shares * _accRewardsPerShare / 1e18 - bondPaid[_id][bond.asset];
242:         }
243:     }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Consider:

• add some variable to track the expired bond shares and exclude these portion in the distribution calculation.
• noted that in extendLock(), the "expired" part also need to taken care of.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L113

Vulnerability details

Impact

According to Chainlink's documentation, the latestAnswer() function is deprecated.

The following could happen:

• Deprecated API stops working. Prices cannot be obtained, if Chainlink stopped supporting deprecated APIs.
• Protocol stops and contracts have to be redeployed.

Proof of Concept

File: contracts/utils/TradingLibrary.sol
113:             int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();

Reference:

Chainlink - latestanswer

Recommended Mitigation Steps

Use the latestRoundData() function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:

    (uint80 roundID, int256 assetChainlinkPriceInt, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
    require(answeredInRound >= roundID, "...");
    require(timeStamp != 0, "...");
    if (assetChainlinkPriceInt != 0) {
        // ...