code-423n4 / 2021-04-basedloans-findings

0 stars 1 forks source link

uint[] memory parameter is tricky #12

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

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.

ghoul-sol commented 3 years ago

As mentioned, this applies to either admin functions or ones that are using a loop. The propose function is used by governance so its outcome will be tested on forked network before voting. I don't see an immediate threat from this solidity bug but we'll keep it in mind.