Tigris Trade contest - wait's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Position.sol#L131-L161

Vulnerability details

Impact

Both the contracts of Position and Trading may not work correctly.

Proof of Concept

The Position.sol#mint calls _safeMint will trigger a _checkOnERC721Received callback, which can be used to reenter.

Crackers can use this vulnerability to attack the protocol.

For example, the cracker could send a reentrant attack tx with a call stack like this:

1. cracker calls his own contract Cx.
2. in Cx: calls Trading.sol#initiateMarketOrder
3. in Trading: calls Position.sol#mint
4. in Position: calls Cx#_checkOnERC721Received
5. in Cx: calls Trading.sol#liquidatePosition (reenter here)
6. in Trading: calls Position.sol#burn
7. in Cx: calls Cx#initiateLimitOrder
8. in Trading: calls Position.sol#mint
9. in Position: calls Cx#_checkOnERC721Received

In step 6, it burned the Position NFT that created in step 3, and the state will be updated in a confusing way because the burnt NFT hasn't done state initialization in step 3 yet. In step 8, it created another Position NFT with the same tokenId as the one in step 3, which is also an anomaly. After step 9, when the stack returns to step 3, the burnt NFT will now go on the state initialization.

Tools Used

Manual

Recommended Mitigation Steps

Simplest solution: replace _safeMint with _mint.

Lines of code

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

Vulnerability details

Impact

All users may lose all their assets in StableVault

Proof of Concept

Owner can drain all the valuable assets in the StableVault easily and quickly like this:

1. Create a mock ERC20 token
2. Mint a lot of the mock token, e.g. 10^15 * 10^18
3. Call StableVault.sol#listToken to list the mock token
4. Deposit a lot of the mock token to the vault
5. Call StableVault.sol#withdraw drain all the valuable assets in the vault. Each asset needs to be called only once.

Tools Used

Manual

Recommended Mitigation Steps

All assets supported by the vault should be fixed at construction time and can not be modified. The owner will not be able to list any new token after the StableVault is deployed. If there is a real need, consider deploying a new StableVault contract.

Lines of code

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

Vulnerability details

Impact

Some users will lose some rewards, and malicious users can use this to gain more rewards.

Proof of Concept

In function BondNFT.sol#claim(), if the claiming bond is expired, the excess of its rewards is re-distributed to all other users:

function claim(
    uint _id,
    address _claimer
) public onlyManager() returns(uint amount, address tigAsset) {
    Bond memory bond = idToBond(_id);
    require(_claimer == bond.owner, "!owner");
    amount = bond.pending;
    tigAsset = bond.asset;
    unchecked {
        if (bond.expired) {
            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]);
            if (totalShares[bond.asset] > 0) {
                accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
            }
        }
        bondPaid[_id][bond.asset] += amount;
    }
    IERC20(tigAsset).transfer(manager, amount);
    emit ClaimFees(tigAsset, amount, _claimer, _id);
}

The following table illustrates the states and transitions of an BondNFT:

• s for shares
• r for reward tokens
• u for unclaimed reward tokens (the pending)
• c for claimed reward tokens

Let's use the above table as an example to analyze the problems.

1. Miscalculation of accRewardsPerShare

The _pendingDelta is the rewards to be re-distributed, and the formula is:

accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];

This formula is wrong. Because the totalShares[bond.asset] includes the expired shares of the claiming bond, which should not be included.

For example, in h7 of the above table claim(b1) is called:

• the _pendingDelta = 20u is re-distributed
• the totalShares = 300s (each of b1, b3, b4 has 100s)

Thus, accRewardsPerShare will increase by _pendingDelta/totalShares = 20u/300s = 0.06666667, which results in the pending reward of b3 and b4 increasing 6.7u each. This is not correct, the 20u/300s should be corrected to 20u/(300s-100s), subtracting shares of b1 from total shares.

i.e. The h7 should be:

2. Unfair re-distribution

The _pendingDelta is re-distributed to all shares of the current epoch. This is not fair for bonds that have been released after the expire time of this claiming bond.

For example, in h7 of the above table claim(b1) is called. The _pendingDelta = 20u is re-distributed to b3 and b4. But in fact it should belong to b2 and b3. Because this _pendingDelta is generated at h3, when b2's share is still there and has not expired, and b4 has not been created yet.

Tools Used

Manual

Recommended Mitigation Steps

The wrong calculation at BondNFT.sol#claim() should be corrected as:

if (totalShares[bond.asset] > bond.shares) {
    accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/(totalShares[bond.asset]-bond.shares);
}

And for the "unfair re-distributed", there seems to be no easy way to fix it. Maybe a scheme like liquidity mining can be considered.