code-423n4 / 2021-05-fairside-findings

0 stars 0 forks source link

Gas optimizations - Use storage instead of memory #53

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

a_delamo

Vulnerability details

Impact

The following methods are using memory instead of storage to read state variables. It is recommended to use storage over memory for gas optimizations.

   // FairSideConviction.sol
    function getConvictionScore(uint256 id)
        public
        view
        returns (uint256 convictionScore)
    {
        //FIXME: Storage
        ConvictionScore memory cs = conviction[id];
        convictionScore = cs.score;

        if (cs.locked != 0)
            convictionScore = convictionScore.add(
                cs.locked.mul(block.timestamp - cs.ts) / 1 days
            );
    }

    function getPriorConvictionScore(address account, uint256 blockNumber)
        public
        view
        override
        returns (uint224)
    {
        require(
            blockNumber < block.number,
            "ERC20ConvictionScore::getPriorConvictionScore: not yet determined"
        );

        uint32 nCheckpoints = numCheckpoints[account];
        if (nCheckpoints == 0) {
            return 0;
        }

        // First check most recent balance
        if (checkpoints[account][nCheckpoints - 1].fromBlock <= blockNumber) {
            return checkpoints[account][nCheckpoints - 1].convictionScore;
        }

        // Next check implicit zero balance
        if (checkpoints[account][0].fromBlock > blockNumber) {
            return 0;
        }

        uint32 lower = 0;
        uint32 upper = nCheckpoints - 1;
        while (upper > lower) {
            uint32 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

            //FIXME: Storage
            Checkpoint memory cp = checkpoints[account][center];
            if (cp.fromBlock == blockNumber) {
                return cp.convictionScore;
            } else if (cp.fromBlock < blockNumber) {
                lower = center;
            } else {
                upper = center - 1;
            }
        }
        return checkpoints[account][lower].convictionScore;
    }

   // FSDNetwork.sol
    function openCostShareRequest(uint256 ethAmount, bool inStable) external {
        //FIXME: storage
        Membership memory user = membership[msg.sender];
        // 90% of the full claim is paid out as 10% is the USA
        uint256 requestPayout = ethAmount.wmul(NON_USA);

        require(
            user.creation + 24 hours <= block.timestamp &&
                user.gracePeriod >= block.timestamp,
            "FSDNetwork::openCostShareRequest: Ineligible cost share request"
        );
        require(
            user.availableCostShareBenefits - user.openCostShareBenefits >=
                requestPayout,
            "FSDNetwork::openCostShareRequest: Cost request exceeds available cost share benefits"
        );

        uint256 fsdSpotPrice = getFSDPrice();
        // We want 10% of the 4% membership fee -> 0.4%
        uint256 bounty =
            user.availableCostShareBenefits.wmul(MEMBERSHIP_FEE / 10).wdiv(
                fsdSpotPrice
            );

        uint256 id = nextCSRID++;

        //FIXME: Use storage over assignation
        costShareRequests[id] = CostShareRequest(
            msg.sender,
            requestPayout,
            bounty,
            uint128(block.timestamp),
            0,
            bytes32(0),
            0
        );
        //FIXME: Re-use user instance
        membership[msg.sender].openCostShareBenefits = user
            .openCostShareBenefits
            .add(requestPayout);

        if (inStable) {
            uint256 etherPrice = getEtherPrice();
            // 5% slippage protection
            uint256 dai =
                fsd.liquidateEth(
                    requestPayout,
                    (requestPayout.mul(etherPrice) / 1 ether).mul(
                        100 - slippageTolerance
                    ) / 100
                );
            costShareRequests[id].stableAmount = dai;
        } else {
            totalOpenRequests = totalOpenRequests.add(requestPayout);
        }

        uint256 halfBounty = bounty / 2;
        // 50% locked
        fsd.safeTransferFrom(msg.sender, address(this), halfBounty);
        // 50% sent to DAO for manual unlocking based on offchain tracking
        fsd.safeTransferFrom(
            msg.sender,
            GOVERNANCE_ADDRESS,
            bounty - halfBounty
        );
    }

   // FSDNetwork.sol
   function updateCostShareRequest(
        uint256 id,
        Action action,
        bytes memory data
    ) external {
        //FIXME: Storage
        CostShareRequest storage csrData = costShareRequests[id];
        require(
            msg.sender == GOVERNANCE_ADDRESS || msg.sender == TIMELOCK,
            "FSDNetwork::updateCostShareRequest: Insufficient Privileges"
        );
        if (action == Action.Expire) {
            require(
                csrData.creation + 7 days <= block.timestamp &&
                    csrData.votingOpen == 0,
                "FSDNetwork::updateCostShareRequest: Invalid claim expiration"
            );
            _processCostShareRequest(
                id,
                csrData.initiator,
                csrData.ethAmount,
                csrData.stableAmount,
                false
            );
        } else if (
            action == Action.SubmitEvidence || action == Action.ExtendClaim
        ) {
            //FIXME: Re-use csrData instance
            CostShareRequest storage csr = costShareRequests[id];
            csr.votingOpen = uint128(block.timestamp);
            if (action == Action.SubmitEvidence) {
                // Should represent hash of evidence
                csr.evidence = abi.decode(data, (bytes32));
            }
        } else {
            _processCostShareRequest(
                id,
                csrData.initiator,
                csrData.ethAmount,
                csrData.stableAmount,
                action == Action.ApproveClaim
            );
        }
    }
fairside-core commented 3 years ago

I am not sure this is applicable, it is actually recommended to use memory over storage when all members are read.

cemozerr commented 3 years ago

Invalid issue as fairside-core's comment on using memory over storage variables for gas optimizations is correct.