Skip to content

Instantly share code, notes, and snippets.

@Dexaran
Last active Nov 2, 2021
Embed
What would you like to do?

Abstract

Review of the IDO contract: https://github.com/SoyFinance/smart-contracts/blob/ea172c78fb817600e6bca9e4919dc07d74646465/IDO/ido.sol

Performed by @Dexaran on 11/2/2021

Findings

1. Ownership assignment is commented out by default (Low)

Code snippet: https://github.com/SoyFinance/smart-contracts/blob/ea172c78fb817600e6bca9e4919dc07d74646465/IDO/ido.sol#L24-L28

Owner address is not assigned to any address by default. The contractr assigns owner upon initialization however: https://github.com/SoyFinance/smart-contracts/blob/ea172c78fb817600e6bca9e4919dc07d74646465/IDO/ido.sol#L248

In theory a third party interferer can call the initialization function to disrupt the deployment process.

2. Usage of loops (Low)

Code snippet: https://github.com/SoyFinance/smart-contracts/blob/ea172c78fb817600e6bca9e4919dc07d74646465/IDO/ido.sol#L337

Usage of loops can cause high gas consumption. The contract does not allow user to specify how much iterations it should perform (claimBehalf function always works from round 1 to currentRound) therefore it is possible that the contract will end in an unusable state where no one can claim their tokens.

This does not pose any significant threat if the total number of rounds is preliminarily known and it calculations show that function call will not exceed the gasLimit.

3. usdValue of a user is never zeroed out (Note/ Indirect logical expressions)

Code snippet: https://github.com/SoyFinance/smart-contracts/blob/ea172c78fb817600e6bca9e4919dc07d74646465/IDO/ido.sol#L338

The logical flow of the contract suggests that bets[i][user].usdValue stores the value (in USD) contributed by a user during a specified round i. This value is later used in calculations of how much SOY reward the user must receive. However the value of this variable is not zeroed out even when the user is already paid (within this function)

It is assumed that the value of lockedUntil (here while non-zero) will prevent the user from invoking the reward calculation function twice for a round that was already paid.

This does not pose any threat in the current state of the contract but in case the logic of lockedUntil is changed in future updates this may potentially break the function that pays rewards.

It may be reasonable to zero out the value of bets[i][user].usdValue once reward is paid.

4. allowedToken mapping is not assigned (High)

Code snippet:

There are requirements for tokens to be market as "allowed" in order to be accepted as payment by the IDO contract. However nobody can assign an "allowed" status to any token.

5. It is not clear what settings the contract should have for weekly / daily auctions (Note/ Documentation requirement)

It is assumed that the contract can be configured to be either (1) daily auction or (2) weekly auction. This same source code will be used to deploy both versions.

@yuriy77k
Copy link

yuriy77k commented Nov 2, 2021

  1. The IDO contract is used with proxy, so it shouldn't use values assigned in constructors.
    On proxy deploying the parameter data set to 0x8129fc1c to call function initialize() to protect from third-party initializtion.

  2. We may use a smaller loop by setting fromRound and toRound to a smaller range.

  3. bets[i][user].usdValue used as a marker of a round where users participate to reduce gas usage on reading an additional value from storage (it will save about 500 gas on each interaction).

  4. Fixed.

  5. Documentation:
    https://docs.google.com/document/d/1eouJkYGE-wtWhccAYaHCHdB8sLnEJ9N154ddMPYNzjc/edit

https://docs.google.com/spreadsheets/d/1eH67f_y1Tvx3HxzpJ8Fk2gFFI3HBBhkaRG7mdQa7tx4/edit#gid=435322608

@yuriy77k
Copy link

yuriy77k commented Nov 2, 2021

According to pt.3, the functions may be like these (but I prefer not to fix it to reduce gas usage):

    // claim SOY tokens from numbers of auction rounds (fromRound to toRound). toRound is excluded.
    function claimBehalf(address user, uint256 fromRound, uint256 toRound) public nonReentrant {
        require(fromRound < toRound && toRound <= currentRoundId, "Incorrect rounds parameters");
        uint256 soyToClaim;
        uint256 _lockPercentage = lockPercentage;
        uint256 _lockPeriod = lockPeriod;
        for (uint256 i = fromRound; i<toRound; i++) {
            uint256 usdValue = bets[i][user].usdValue;
            if (usdValue != 0) { // user contributed in this round
                if (bets[i][user].lockedUntil == 0) { // receive token from round
                    uint256 total = auctionRound[i].soyToSell * usdValue / auctionRound[i].usdCollected;
                    uint256 locked = total * _lockPercentage / 100;
                    soyToClaim += (total - locked);
                    bets[i][user].soyAmount = locked;
                    bets[i][user].lockedUntil = auctionRound[i].end + _lockPeriod;
                    bets[i][user].usdValue = 0;
                }
            }
            // Check if can claim locked tokens 
            uint256 soyAmount = bets[i][user].soyAmount;
            if (soyAmount != 0 && bets[i][user].lockedUntil < block.timestamp) {
                soyToClaim += soyAmount;
                bets[i][user].soyAmount = 0;
            }
        }
        IERC223(SoyToken).transfer(user, soyToClaim);
    }

    // return amount of SOY tokens that user may claim and amount that locked
    function getTokenToClaim(address user) external view returns(uint256 soyToClaim, uint256 soyLocked) {
        uint256 toRound = currentRoundId;
        uint256 _lockPercentage = lockPercentage;
        uint256 _lockPeriod = lockPeriod;
        for (uint256 i = 1; i < toRound; i++) {
            uint256 usdValue = bets[i][user].usdValue;
            if (usdValue != 0) { // user contributed in this round
                uint256 lockedUntil = bets[i][user].lockedUntil;
                uint256 locked;
                if (lockedUntil == 0) { // receive token from round
                    uint256 total = auctionRound[i].soyToSell * usdValue / auctionRound[i].usdCollected;
                    locked = total * _lockPercentage / 100;
                    soyToClaim += (total - locked);
                    lockedUntil = auctionRound[i].end + _lockPeriod;
                }
                if (lockedUntil < block.timestamp) {
                    soyToClaim += bets[i][user].soyAmount;
                    soyToClaim += locked;   // in case of user do first claim in 1 year after auction end.
                } else {
                    soyLocked += bets[i][user].soyAmount;
                }
            } else if (bets[i][user].soyAmount != 0){
                if (bets[i][user].lockedUntil < block.timestamp) {
                    soyToClaim += bets[i][user].soyAmount;
                } else {
                    soyLocked += bets[i][user].soyAmount;
                }
            }

        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment