yAxis contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Proof of Concept

There is no need to cache calculation steps between the return values. L98-L122 is as follows:

        uint256 _endingBalance = _token.balanceOf(_recipient);
        uint256 _withdrawnAmount = _token.balanceOf(_recipient).sub(_startingBalance);

        uint256 _endingTotalValue = _self.totalValue();
        uint256 _decreasedValue = _startingTotalValue.sub(_endingTotalValue);

Which can be replaced by following code to save gas:

        uint256 _withdrawnAmount = _token.balanceOf(_recipient).sub(_startingBalance);

        uint256 _decreasedValue = _startingTotalValue.sub(_self.totalValue());

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

L839-L843 is as follow:

            AlchemistVault.Data storage _activeVault = _vaults.last();
            (uint256 _withdrawAmount, uint256 _decreasedValue) = _activeVault.withdraw(
                _recipient,
                _remainingAmount
            );

It can be replaced by following code block, since there is no reason to save it to memory.

            (uint256 _withdrawAmount, uint256 _decreasedValue) = _vaults.last().withdraw(
                _recipient,
                _remainingAmount
            );

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {
//Operations not effecting the length of the array.
}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Recommended implementation:

uint length = arr.length;
for (uint i = 0; i < length; i++) {
//Operations not effecting the length of the array.
}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

./controllers/Controller.sol:453:        for (uint i = 0; i < _strategies.length; i++) {
./converters/GeneralConverter.sol:109:            for (uint8 i = 0; i < tokens.length; i++) {
./converters/GeneralConverter.sol:137:            for (uint8 i = 0; i < tokens.length; i++) {
./converters/GeneralConverter.sol:174:            for (uint8 i = 0; i < tokens.length; i++) {
./converters/GeneralConverter.sol:189:            for (uint8 i = 0; i < tokens.length; i++) {

Handle

0x0x0x

Vulnerability details

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Occurences

./Alchemist.sol:395:        if (_harvestedAmount > 0) {
./Alchemist.sol:404:            if (_feeAmount > 0) {
./Alchemist.sol:408:            if (_distributeAmount > 0) {
./Alchemist.sol:556:        if (_parentAmount > 0) {
./Alchemist.sol:561:        if (_childAmount > 0) {
./Alchemist.sol:627:            if (borrowFee > 0) {
./Alchemist.sol:729:        if (pegMinimum > 0) {
./Alchemist.sol:839:        if (_remainingAmount > 0) {
./Transmuter.sol:115:        if (owing > 0) {
./Transmuter.sol:140:        if (_buffer > 0) {
./Transmuter.sol:160:            if(_toDistribute > 0){
./Transmuter.sol:202:        require(realisedTokens[sender] > 0);
./Transmuter.sol:247:        require(pendingz > 0, "need to have pending in bucket");
./Transmuter.sol:322:        if (realisedTokens[toTransmute] > 0) {
./Transmuter.sol:372:        if(totalSupplyAltokens > 0 && amount > 0) {

Tools Used

grep

Handle

0x0x0x

Vulnerability details

Impact

Alchemist.sol#acceptGovernance(L216-225) is:

    function acceptGovernance() external {
        require(msg.sender == _pendingGovernance, 'sender is not pendingGovernance');
        address _pendingGovernance = pendingGovernance;
        governance = _pendingGovernance;

        emit GovernanceUpdated(_pendingGovernance);
    }

It can be replaced with following code to save gas:

    function acceptGovernance() external {
        address _pendingGovernance = pendingGovernance;
        require(msg.sender == _pendingGovernance, 'sender is not pendingGovernance');
        governance = _pendingGovernance;

        emit GovernanceUpdated(_pendingGovernance);
    }

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Concept

The current code is:

  function update(Data storage _self, Context storage _ctx) internal {
    uint256 _earnedYield = _self.getEarnedYield(_ctx);
    if (_earnedYield > _self.totalDebt) {
      uint256 _currentTotalDebt = _self.totalDebt;
      _self.totalDebt = 0;
      _self.totalCredit = _earnedYield.sub(_currentTotalDebt);
    } else {
      _self.totalDebt = _self.totalDebt.sub(_earnedYield);
    }
    _self.lastAccumulatedYieldWeight = _ctx.accumulatedYieldWeight;
  }

We cache _self.totalDebt, but it is not required, since we can use it before we change it. This code block can be replaced with:

  function update(Data storage _self, Context storage _ctx) internal {
    uint256 _earnedYield = _self.getEarnedYield(_ctx);
    if (_earnedYield > _self.totalDebt) {
      _self.totalCredit = _earnedYield.sub(_self.totalDebt);
      _self.totalDebt = 0;
    } else {
      _self.totalDebt = _self.totalDebt.sub(_earnedYield);
    }
    _self.lastAccumulatedYieldWeight = _ctx.accumulatedYieldWeight;
  }

By doing so, we don't cache _self.totalDebt just to use it once.

Handle

0x0x0x

Vulnerability details

Proof of Concept

Current implementation has two if statements, but actually the same logic can be coded with only one if statement. Since _unclaimedYield == 0 is a special case of _unclaimedYield < _currentTotalDebt and does not require any extra code.

  function getUpdatedTotalDebt(Data storage _self, Context storage _ctx) internal view returns (uint256) {
    uint256 _unclaimedYield = _self.getEarnedYield(_ctx);
    uint256 _currentTotalDebt = _self.totalDebt;

    if (_unclaimedYield < _currentTotalDebt) {
      return _currentTotalDebt - _unclaimedYield;
    }
    else {
      return 0;
    }
  }

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

At YearnVaultAdapter.sol, we have vault, admin and decimals and at YaxisVaultAdapter.sol, we have vault and admin as global variables. They are set at constructor and never changed again. Therefore, it saves gas by making them immutable. It also increases code readibilty, since immutable variables are guaranteed to be constant after constructor.

Tools Used

Manual analysis