Based Loans contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

Using memory array parameters (e.g. uint[] memory) as function parameters can be tricky in Solidity, because an attack is possible with a very large array which will overlap with other parts of the memory. See proof of concept below.

The function "propose" of GovernorAlpha.sol seems most vulnerable because this function does not check the validity of the array lengths. Most other functions do a loop over the array, which will fail with a large array (due to out of gas).

Proof of Concept

The following functions use a [] memory parameter. .\Comptroller.sol: function enterMarkets(address[] memory cTokens) public override returns (uint[] memory) { .\Comptroller.sol: function claimComp(address holder, CToken[] memory cTokens) public { .\Comptroller.sol: function claimComp(address[] memory holders, CToken[] memory cTokens, bool borrowers, bool suppliers) public { .\Comptroller.sol: function _addCompMarkets(address[] memory cTokens) public { .\Governance\GovernorAlpha.sol: function propose(address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas, string memory description) public returns (uint) { .\UniswapOracle\UniswapAnchoredView.sol: function addTokens(TokenConfig[] memory configs) public onlyOwner { .\UniswapOracle\UniswapConfig.sol: function _addTokensInternal(TokenConfig[] memory configs) internal {

This an example to show the exploit: // based on https://github.com/paradigm-operations/paradigm-ctf-2021/blob/master/swap/private/Exploit.sol pragma solidity ^0.4.24; // only works with low solidity version

contract test{ struct Overlap { uint field0; } event log(uint);

function mint(uint[] memory amounts) public returns (uint) { // this can be in any solidity version Overlap memory v; v.field0 = 1234; emit log(amounts[0]); // would expect to be 0 however is 1234 return 1; }

function go() public { // this part requires the low solidity version uint x=0x800000000000000000000000000000000000000000000000000000000000000; // 2^251 bytes memory payload = abi.encodeWithSelector(this.mint.selector, 0x20, x); bool success=address(this).call(payload); } }

Tools Used

Editor

Recommended Mitigation Steps

Add checks on the size of the array parameters to make sure they are not absurdly long.

Handle

gpersoon

Vulnerability details

Impact

CarefulMath is used in most calculations, however it isn't always used. Although I didn't spot any real issues with this it isn't consistent.

Proof of Concept

.\Governance\Blo.sol: return nCheckpoints > 0 ? checkpoints[account][nCheckpoints - 1].votes : 0; .\Governance\Blo.sol: if (checkpoints[account][nCheckpoints - 1].fromBlock <= blockNumber) { .\Governance\Blo.sol: return checkpoints[account][nCheckpoints - 1].votes; .\Governance\Blo.sol: uint32 upper = nCheckpoints - 1; .\Governance\Blo.sol: upper = center - 1; .\Governance\Blo.sol: uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0; .\Governance\Blo.sol: uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0; .\Governance\Blo.sol: if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) { .\Governance\Blo.sol: checkpoints[delegatee][nCheckpoints - 1].votes = newVotes; .\CToken.sol: totalReservesNew = totalReserves - reduceAmount; .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - newObservation.timestamp; .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - oldTimestamp; .\UniswapOracle\UniswapAnchoredView.sol: FixedPoint.uq112x112 memory priceAverage = FixedPoint.uq112x112(uint224((nowCumulativePrice - oldCumulativePrice) / timeElapsed)); .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - newObservation.timestamp; .\CEther.sol: fullMessage[i+2] = byte(uint8(48 + ( errCode / 10 ))); .\CEther.sol: fullMessage[i+3] = byte(uint8(48 + ( errCode % 10 ))); .\DAIInterestRateModelV3.sol: gapPerBlock = 4e16 / blocksPerYear; .\DAIInterestRateModelV3.sol: gapPerBlock = gapPerYear / blocksPerYear; .\UniswapOracle\UniswapAnchoredView.sol: return mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: price = mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(1e30, price) / config.baseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(1e30, priceInternal(config)) / config.baseUnit; .\UniswapOracle\UniswapAnchoredView.sol: FixedPoint.uq112x112 memory priceAverage = FixedPoint.uq112x112(uint224((nowCumulativePrice - oldCumulativePrice) / timeElapsed)); .\UniswapOracle\UniswapAnchoredView.sol: anchorPrice = mul(unscaledPriceMantissa, config.baseUnit) / ethBaseUnit / expScale; .\UniswapOracle\UniswapLib.sol: return uq112x112((uint224(numerator) << 112) / denominator); .\UniswapOracle\UniswapLib.sol: return uint(self._x) / 5192296858534827; .\UniswapOracle\UniswapLib.sol: return uint32(block.timestamp % 2 ** 32); .\UniswapOracle\UniswapLib.sol: uint32 timeElapsed = blockTimestamp - blockTimestampLast; .\UniswapOracle\UniswapLib.sol: price0Cumulative += uint(FixedPoint.fraction(reserve1, reserve0)._x) * timeElapsed; .\UniswapOracle\UniswapLib.sol: price1Cumulative += uint(FixedPoint.fraction(reserve0, reserve1)._x) * timeElapsed;

Tools Used

grep

Recommended Mitigation Steps

Double check to see if safe math functions really are not necessary and otherwise add a comment

Handle

gpersoon

Vulnerability details

Impact

The function requireNoError of Cether.sol contains 2 checks on errCode == uint(Error.NO_ERROR). After the first check it returns. After this errCode == uint(Error.NO_ERROR) will never be true, so doesn't have to be checked.

Proof of Concept

function requireNoError(uint errCode, string memory message) internal pure { if (errCode == uint(Error.NO_ERROR)) { return; } ... require(errCode == uint(Error.NO_ERROR), string(fullMessage));

Tools Used

Editor

Recommended Mitigation Steps

Replace require(errCode == uint(Error.NO_ERROR), string(fullMessage)); with require(false, string(fullMessage));

Note: Solidity 8.4 has new error handling functionality which could replace the logic of requireNoError

Handle

gpersoon

Vulnerability details

Impact

The compound contracts have recently added the function sweepToken to CErc20.sol This function isn't present in the basedloans contract See: https://github.com/compound-finance/compound-protocol/commit/b198cb4dac977c61fa793ffe441c932438e83cdc

Proof of Concept

https://github.com/compound-finance/compound-protocol/blob/master/contracts/CErc20.sol#L116

function sweepToken(EIP20NonStandardInterface token) external { require(address(token) != underlying, "CErc20::sweepToken: can not sweep underlying token"); uint256 balance = token.balanceOf(address(this)); token.transfer(admin, balance); }

Tools Used

diff

Recommended Mitigation Steps

It might be useful to also add the function sweepToken