Yield contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function stir of Cauldron.sol can be manipulated when from == to. In that case the balance of "to" is increased while the balance of "from" isn't decreased. This is due to the fact that a temporary variable is used and the balance of "to" overwrites the balance of "from".

Below is proof of concept with a simplified version of stir which shows the issue. The initial balance of 20 is increased to 25 without decreasing another balance.

Proof of Concept

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Cauldron.sol#L268 contract test {

struct Balances {
    uint128 art;          // Debt amount
    uint128 ink;          // Collateral amount
}
mapping (uint => Balances) public balances; 
constructor() {
    balances[0]=Balances(20,20);
}
function stir(uint from, uint to, uint128 ink, uint128 art) public
{
    Balances memory balancesFrom = balances[from];
    Balances memory balancesTo = balances[to];
    if (ink > 0) {
        balancesFrom.ink -= ink;
        balancesTo.ink += ink;
    }
    if (art > 0) {
        balancesFrom.art -= art;
        balancesTo.art += art;
    }
    balances[from] = balancesFrom;
    balances[to] = balancesTo;

} function TestStir() public returns(Balances memory) { stir(0,0,5,5); return balances[0]; // returns (25,25)
} }

Tools Used

Editor

Recommended Mitigation Steps

Add a check to prevent that to and from are the same, for example require (to != from, "To and From should be different");

Handle

gpersoon

Vulnerability details

Impact

The auth mechanism of AccessControl.sol uses function selectors (msg.sig) as a (unique) role definition. Also the _moduleCall allows the code to be extended. Suppose an attacker wants to add the innocent looking function "left_branch_block(uint32)" in an new module. Suppose this module is added via _moduleCall and the attacker gets authorization for the innocent function. This functions happens to have a signature of 0x00000000, which is equal to the root authorization. This way the attacker could get authorization for the entire project.

Note: it's pretty straightforward to generate function names for any signature value, you can just brute force it because it's only 4 bytes.

Proof of Concept

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/utils/access/AccessControl.sol#L90 modifier auth() { require (_hasRole(msg.sig, msg.sender), "Access denied"); _; }

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Ladle.sol#L588 function _moduleCall(address module, bytes memory moduleCall) private returns (bool success, bytes memory result) { require (modules[module], "Unregistered module"); (success, result) = module.delegatecall(moduleCall); if (!success) revert(RevertMsgExtractor.getRevertMsg(result)); } }

// https://www.4byte.directory/signatures/?bytes4_signature=0x00000000 Text Signature Bytes Signature left_branch_block(uint32) 0x00000000

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/utils/access/AccessControl.sol#L47 bytes4 public constant ROOT = 0x00000000;

Tools Used

Recommended Mitigation Steps

Do not allow third parties to define or suggest new modules Double check the function signatures of new functions of a new module for collisions

Handle

gpersoon

Vulnerability details

Impact

The V1 version of YieldMath.sol contains ">=" (larger or equal), while the V2 version of YieldMath.sol containt ">" (larger) in the log_2 function. This change doesn't seem logical and might lead to miss calculations. The difference is present in a number of adjacent lines.

Proof of Concept

// https://github.com/yieldprotocol/yieldspace-v1/blob/master/contracts/YieldMath.sol#L217 function log_2 (uint128 x) ... b = b * b >> 127; if (b >= 0x100000000000000000000000000000000) {b >>= 1; l |= 0x1000000000000000000000000000000;}

//https://github.com/code-423n4/2021-05-yield/blob/main/contracts/yieldspace/YieldMath.sol#L58 function log_2(uint128 x) ... b = b * b >> 127; if(b > 0x100000000000000000000000000000000) {b >>= 1; l |= 0x1000000000000000000000000000000;}

Tools Used

diff

Recommended Mitigation Steps

Check which version is the correct version and fix the incorrect version.

Handle

gpersoon

Vulnerability details

Impact

The witch.sol contract gets access to a vault via the grab function, in case of liquidation. If the witch.sol contract can't sell the debt within a certain amount of time, a second grab can occur.

After the second grab, the information of the original owner of the vault is lost and the vault can't be returned to the original owner once the debt has been sold.

The grab function stores the previous owner in vaultOwners[vaultId] and then the contract itself is the new owner (via cauldron.grab and cauldron._give). The vaultOwners[vaultId] is overwritten at the second grab

The function buy of Witch.sol tried to give the vault back to the original owner, which won't succeed after a second grab.

Proof of Concept

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Witch.sol#L50 function grab(bytes12 vaultId) public { DataTypes.Vault memory vault = cauldron.vaults(vaultId); vaultOwners[vaultId] = vault.owner; cauldron.grab(vaultId, address(this)); }

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Cauldron.sol#L349 function grab(bytes12 vaultId, address receiver) external auth { ... _give(vaultId, receiver);

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Cauldron.sol#L349 function _give(bytes12 vaultId, address receiver) internal returns(DataTypes.Vault memory vault) { ... vault.owner = receiver; vaults[vaultId] = vault;

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/Witch.sol#L57 function buy(bytes12 vaultId, uint128 art, uint128 min) public { .... cauldron.give(vaultId, vaultOwners[vaultId]);

Tools Used

Editor

Recommended Mitigation Steps

Assuming it's useful to give back to vault to the original owner: Make a stack/array of previous owners if multiple instances of the witch.sol contract would be used. Or check if the witch is already the owner (in the grab function) and keep the vaultOwners[vaultId] if that is the case

Handle

gpersoon

Vulnerability details

Impact

The auth modifier of AccessControl.sol doesn't work as you would expect. It checks if you are authorized for "msg.sig", however msg.sig is the signature of the first function you have called, not of the current function. So if you call function A, which calls function B, the "auth" modifier of function B checks if you are authorized for function A!

There is a difference between external an public functions. For external functions this works as expected because a fresh call (with a new msg.sig) is always made. However with a public functions, which are called from within the same contract, this doesn't happen and the problem described above occurs. See in the proof of concept for a piece of code which shows the problem. In the code there are several functions which have public and auth combined, see also in the proof of concept .

In the current codebase I couldn't find a problem situation, however this could be accidentally introduced with future changes. If could also be introduced via the _moduleCall of Ladle.sol, which allows functions to be defined which might call the public functions.

Proof of Concept

auth

// https://github.com/code-423n4/2021-05-yield/blob/main/contracts/utils/access/AccessControl.sol#L90 modifier auth() { require (_hasRole(msg.sig, msg.sender), "Access denied"); _; }

example

pragma solidity ^0.8.0; contract TestMsgSig { event log(bytes4);

function setFeePublic(uint256) public  {
     emit log(this.setFeePublic.selector);
     emit log(msg.sig);
}
function setFeeExternal(uint256) external  {
     emit log(this.setFeeExternal.selector);
     emit log(msg.sig);
}

function TestPublic() public {
    setFeePublic(2);
}

function TestExternal() public {
   this.setFeeExternal(2);
}

}

occurrences of public auth

Wand.sol: function addAsset(bytes6 assetId,address asset) public auth { Wand.sol: function makeBase(bytes6 assetId, IMultiOracleGov oracle, address rateSource, address chiSource) public auth { Wand.sol: function makeIlk(bytes6 baseId, bytes6 ilkId, IMultiOracleGov oracle, address spotSource, uint32 ratio, uint96 max, uint24 min, uint8 dec) public auth { Wand.sol: function addSeries(... ) public auth { Witch.sol: function setAuctionTime(uint128 auctionTime_) public auth { Witch.sol: function setInitialProportion(uint128 initialProportion_) public auth { Ladle.sol: function setFee(uint256 fee) public auth
Join.sol: function setFlashFeeFactor(uint256 flashFeeFactor_) public auth { oracles\chainlink\ChainlinkMultiOracle.sol: function setSource(bytes6 base, bytes6 quote, address source) public auth { oracles\chainlink\ChainlinkMultiOracle.sol: function setSources(bytes6[] memory bases, bytes6[] memory quotes, address[] memory sources_) public auth { oracles\compound\CompoundMultiOracle.sol: function setSource(bytes6 base, bytes6 kind, address source) public auth { oracles\compound\CompoundMultiOracle.sol: function setSources(bytes6[] memory bases, bytes6[] memory kinds, address[] memory sources_) public auth { oracles\uniswap\UniswapV3Oracle.sol: function setSecondsAgo(uint32 secondsAgo_) public auth { oracles\uniswap\UniswapV3Oracle.sol: function setSource(bytes6 base, bytes6 quote, address source) public auth { oracles\uniswap\UniswapV3Oracle.sol: function setSources(bytes6[] memory bases, bytes6[] memory quotes, address[] memory sources_) public auth { fytoken.sol: function setOracle(IOracle oracle_) public auth {

Tools Used

grep

Recommended Mitigation Steps

make sure all auth functions use external (still error prone) or change the modifier to something like:

modifier auth(bytes4 fs) { require (msg.sig == fs,"Wrong selector"); require (_hasRole(msg.sig, msg.sender), "Access denied"); _; }

function setFee(uint256) public auth(this.setFee.selector) {
   .....
}